Review of patch to avoid adding magic with rvalue $#a (XS gurus wanted)

ikegami has asked for the wisdom of the Perl Monks concerning the following question:

$#a adds magic to the associated array. This costs time and memory. I've written a patch to prevent that when $#a is used as an rvalue.

speed_test.pl:

use strict;
use warnings;

use Devel::Peek qw( Dump );
use Time::HiRes qw( time );

my @a = qw( a b c d e f g h i j );

my $stime = time;
my $x; $x = $#a for 1..1_000_000;
my $etime = time;

print("time delta = ", $etime-$stime, "\n");

Dump(\@a, 1) if $ARGV[0];
[download]

Before change:

$ ./perl -Ilib speed_test.pl 0
time delta = 0.623134136199951

$ ./perl -Ilib speed_test.pl 0
time delta = 0.621975898742676

$ ./perl -Ilib speed_test.pl 0
time delta = 0.626943826675415

$ ./perl -Ilib speed_test.pl 1
time delta = 0.636697053909302
SV = IV(0x8400afc) at 0x8400b00
  REFCNT = 1
  FLAGS = (TEMP,ROK)
  RV = 0x83ca0f0
  SV = PVAV(0x83bb1b4) at 0x83ca0f0
    REFCNT = 2
    FLAGS = (PADMY,RMG)
    MAGIC = 0x83c5210
      MG_VIRTUAL = &PL_vtbl_arylen_p
      MG_TYPE = PERL_MAGIC_arylen_p(@)
      MG_FLAGS = 0x02
        REFCOUNTED
      MG_OBJ = 0x83ca280
      SV = PVMG(0x840f42c) at 0x83ca280
        REFCNT = 1
        FLAGS = (GMG,SMG,pIOK)
        IV = 9
        NV = 0
        PV = 0
        MAGIC = 0x83c5230
          MG_VIRTUAL = &PL_vtbl_arylen
          MG_TYPE = PERL_MAGIC_arylen(#)
          MG_OBJ = 0x83ca0f0
    ARRAY = 0x83c5180
    FILL = 9
    MAX = 9
    ARYLEN = 0x83ca280
    FLAGS = (REAL)
[download]

After change:

$ ./perl -Ilib speed_test.pl 0
time delta = 0.463121175765991

$ ./perl -Ilib speed_test.pl 0
time delta = 0.498654127120972

$ ./perl -Ilib speed_test.pl 0
time delta = 0.462605953216553

$ ./perl -Ilib speed_test.pl 1
time delta = 0.464058876037598
SV = IV(0x83cb27c) at 0x83cb280
  REFCNT = 1
  FLAGS = (TEMP,ROK)
  RV = 0x83cb0f0
  SV = PVAV(0x83bc1b4) at 0x83cb0f0
    REFCNT = 2
    FLAGS = (PADMY)
    ARRAY = 0x83c6180
    FILL = 9
    MAX = 9
    ARYLEN = 0x0
    FLAGS = (REAL)
[download]

Unfortunately, I'm not very confident about my patch

From c045b931b2afb3b2af6641a9943f65f295ee7da1 Mon Sep 17 00:00:00 2001
From: Eric Brine <ikegami@adaelis.com>
Date: Fri, 23 Oct 2009 19:05:40 -0400
Subject: [PATCH] Avoid adding magic with rvalue $#a

---
 pp.c |   17 ++++++++++++-----
 1 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/pp.c b/pp.c
index 6f56368..b9b8ba6 100644
--- a/pp.c
+++ b/pp.c
@@ -321,12 +321,19 @@ PP(pp_av2arylen)
 {
     dVAR; dSP;
     AV * const av = MUTABLE_AV(TOPs);
-    SV ** const sv = Perl_av_arylen_p(aTHX_ MUTABLE_AV(av));
-    if (!*sv) {
-       *sv = newSV_type(SVt_PVMG);
-       sv_magic(*sv, MUTABLE_SV(av), PERL_MAGIC_arylen, NULL, 0);
+    const I32 lvalue = PL_op->op_flags & OPf_MOD || LVRET;
+    if (lvalue) {
+       SV ** const sv = Perl_av_arylen_p(aTHX_ MUTABLE_AV(av));
+       if (!*sv) {
+           *sv = newSV_type(SVt_PVMG);
+           sv_magic(*sv, MUTABLE_SV(av), PERL_MAGIC_arylen, NULL, 0);
+       }
+       SETs(*sv);
+    } else {
+       SETs(sv_2mortal(newSViv(
+           AvFILL(MUTABLE_AV(av)) + CopARYBASE_get(PL_curcop)
+       )));
     }
-    SETs(*sv);
     RETURN;
 }

--
1.6.5
[download]

Specifically,

Is it ok to define sv inside the curlies?
Is the arg guaranteed to be an AV?
Is the sv_2mortal appropriate?

Also, I initially tried to avoid creating a new SV by using

        dTARGET;
        SETi( AvFILL(MUTABLE_AV(av)) + CopARYBASE_get(PL_curcop) );
[download]

instead of

        SETs(sv_2mortal(newSViv(
            AvFILL(MUTABLE_AV(av)) + CopARYBASE_get(PL_curcop)
        )));
[download]

but I got the following while building the rest of Perl:

panic: pad_sv po at lib/re.pm line 99.
[download]

Is using TARG the wrong way to go, or did I just do it wrong? I based my code on other ops.

Comment on Review of patch to avoid adding magic with rvalue $#a (XS gurus wanted) Select or Download Code

Replies are listed 'Best First'.
Re: Review of patch to avoid adding magic with rvalue $#a (XS gurus wanted) by almut (Canon) on Oct 26, 2009 at 20:16 UTC
Is it ok to define `sv` inside the curlies? That shouldn't be of much concern portability-wise. Variable declarations at the beginning of a block (compound statement) have pretty much always been supported, at least since ANSI/ISO C (aka C89/C90), and - AFAIK - even before that time (i.e. old K&R-like variants). As I wasn't entirely sure about definitions (as opposed to declarations), I tried the following snippet with the oldest compilers I could find (up to 12 years old) on various Unix platforms: `int main() { int x = 42; if (x==42) { int y = 99; x += y; } return 0; }` [download] It did compile fine on all of them. Both in ANSI mode and when explicitly requesting the respective K&R emulation modes, i.e. `cc -qlanglvl=classic` (AIX), `cc -Ac` (HP-UX), `cc -Xs` (SunOS), `cc -cckr` (IRIX), `gcc -traditional` (Linux), etc. (note that `-traditional` is no longer supported with modern versions of `gcc` — I used a 10-year old gcc-2.95) What isn't supported in strict ANSI C is mixing declarations/definitions and statements within the same block, such as `if (x==42) { x++; int y = 99; ... }` [download]	[reply] [d/l] [select]
Re: Review of patch to avoid adding magic with rvalue $#a (XS gurus wanted) by Herkum (Parson) on Oct 26, 2009 at 16:07 UTC
I have been watching this question and hoping someone one respond with an answer on the subject so that I could get more insight. This is a relatively simple question and it has not received a response. I think that this question points out one of the weaknesses of Perl. XS is poorly documented/explained so there are few people who are able to be subject matter experts. I really wish that XS was more accessible so that more people could get involved on the subject.	[reply]
Re^2: Review of patch to avoid adding magic with rvalue $#a (XS gurus wanted) by ikegami (Patriarch) on Oct 26, 2009 at 16:33 UTC
My questions aren't really XS questions. The question about the var declaration is a C portability question. Whether the arg is guaranteed to be an AV or not has nothing to do with XS. It relates to the argument of an internal function that's only called internally. The question about sv_2mortal is an elementary XS question, and it is documented. ("They are made mortal so that once the values are copied by the calling program, the SV's that held the returned values can be deallocated. If they were not mortal, then they would continue to exist after the XSUB routine returned, but would not be accessible. This is a memory leak.") My question was more along the lines of "does that apply to operators as well"? Of course, the answer is yes. Finally, the question about dTARGET is specific to functions that implement perl opcodes. (For XS, you'd add `dXSTARG;` to make it work.) I'll grant you that Perl's internals aren't fully documented.	[reply] [d/l]