Front page | perl.perl5.porters |
Postings from June 2013
Re: [perl #118269] [PATCH] SV_CONST(XXX) - constant strings withprecomputed values
Thread Previous
|
Thread Next
From:
Ruslan Zakirov
Date:
June 30, 2013 10:45
Subject:
Re: [perl #118269] [PATCH] SV_CONST(XXX) - constant strings withprecomputed values
Message ID:
CAMOxC8vOW-sdaz3H_f8-QNzpDiEVqiOWUP2ptTfMpBnGh+L3Sw@mail.gmail.com
On Thu, Jun 27, 2013 at 5:15 AM, Tony Cook via RT <perlbug-followup@perl.org
> wrote:
> On Sat Jun 01 07:15:09 2013, ruz@bestpractical.com wrote:
> > Want to get ball rolling on this.
> >
> > SV_CONST macro. Goal is to use existing optimizations in
> > pp_method_named
> > for TIESCALAR, FETCH, POP and friends by using precomputed hash values
> > of these constant strings. You can find more details on this
> > miniproject
> > in earlier thread [1].
> >
> > Code is on github [2], just repushed the branch. Full diff [3].
> >
> > So far I'm happy with the change and consider it's ready for full
> > review
> > and merge into blead.
> >
> > [1] http://markmail.org/message/khezkunng5ggosys
> > [2] https://github.com/ruz/perl5/commits/ruz/tie-method-calls
> > [3] https://github.com/ruz/perl5/compare/blead...ruz;tie-method-calls
>
> Pulled and rebased on blead (which may change some line numbers),
> patches attached.
>
re-pushed with fixups.
> Running make test fails since op/svleak.t fails.
>
Doesn't fail anymore.
>
> A few questions:
>
> #define G_WRITING_TO_STDERR 1024 /* Perl_write_to_stderr() is calling
> Perl_magic_methcall(). */
> +#define G_METHOD_NAMED 2048+128 /* calling named method, eg
> without ::
> or ' */
> #define G_RE_REPARSING 0x800 /* compiling a run-time /(?{..})/ */
>
> Is the overlap between G_METHOD_NAMED and G_RE_PARSING deliberate?
> (0x800 == 2048). (It doesn't help that Dave changed from decimal to hex.)
>
No, it's not deliberate. While they don't conflict I decided to change
G_METHOD_NAMED to 4096.
If I understand where and how G_RE_PARSING is used the overlap is
> harmless, but it may be confusing to future readers of the code - they
> could assume that G_METHOD_NAMED is intended to combine the effects of
> G_METHOD and G_RE_REPARSING, which I don't think is the case.
>
> If G_METHOD_NAMED is meant to overlap with G_METHOD (and I'm pretty sure
> it is), this #define should use the symbolic name (and use parens, it
> saves pain):
>
> #define G_METHOD_NAMED (2048|G_METHOD) /* calling named method, eg
> without :: or ' */
>
I've looked at my code and decided that there is no need in overlapping with
G_METHOD.
>
> I'd use | instead of + too, to be clear we're dealing with bits.
>
> EXTEND(PL_stack_sp, 1);
> - *++PL_stack_sp = sv;
> + if (!(flags & G_METHOD_NAMED))
> + *++PL_stack_sp = sv;
> oldmark = TOPMARK;
> oldscope = PL_scopestack_ix;
>
> Since G_METHOD_NAMED includes G_METHOD this conditional always fails
> when calling a method, but because of the next chunk, you luck out and
> it works.
>
> if (flags & G_METHOD) {
> - Zero(&method_op, 1, UNOP);
> - method_op.op_next = (OP*)&myop;
> - method_op.op_ppaddr = PL_ppaddr[OP_METHOD];
> - method_op.op_type = OP_METHOD;
> - myop.op_ppaddr = PL_ppaddr[OP_ENTERSUB];
> - myop.op_type = OP_ENTERSUB;
> - PL_op = (OP*)&method_op;
> + if ( flags & G_METHOD_NAMED ) {
> + Zero(&method_svop, 1, SVOP);
>
> G_METHOD_NAMED is 128 + 2048 and G_METHOD is 128, so if flags & G_METHOD
> is true, then the condition for the nested if is also true
>
Lack of constant paractice with bitwise operations. I find new code with
isolated G_METHOD
and G_METHOD_NAMED is easier to read and probably is better way to go.
> +#define SV_CONST(name) \
> + PL_sv_consts[SV_CONST_##name] \
> + ? PL_sv_consts[SV_CONST_##name] \
> + : newSVpv_share(#name, 0)
> +
>
> What actually sets PL_sv_consts[SV_CONST_*] ? I assume the result of
> newSVpv_share() here is leaking, causing the op/svleak.t test to fail.
>
You're right. Last line should have been an assignment.
Code is rebased on blead, fexed up and re-pushed. Thanks a lot for review.
Tony
>
>
> ---
> via perlbug: queue: perl5 status: open
> https://rt.perl.org:443/rt3/Ticket/Display.html?id=118269
>
--
Best regards, Ruslan.
Thread Previous
|
Thread Next