develooper Front page | perl.perl5.porters | Postings from June 2013

[perl #118269] [PATCH] SV_CONST(XXX) - constant strings with precomputed values

Thread Next
From:
Tony Cook via RT
Date:
June 27, 2013 01:15
Subject:
[perl #118269] [PATCH] SV_CONST(XXX) - constant strings with precomputed values
Message ID:
rt-3.6.HEAD-17467-1372295732-976.118269-15-0@perl.org
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.

Running make test fails since op/svleak.t fails.

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.)

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'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.

+#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.

Tony


---
via perlbug:  queue: perl5 status: open
https://rt.perl.org:443/rt3/Ticket/Display.html?id=118269

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