develooper 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


nntp.perl.org: Perl Programming lists via nntp and http.
Comments to Ask Bjørn Hansen at ask@perl.org | Group listing | About