Don't zero non-real arrays

[hvds]

A minor difference in behaviour introduced by the rewrite of av_extend_guts
in 440c185 and 399fef9 is that when the SV* array is first acquired, we
now zero its contents regardless of whether that is needed - and add an
extra label and 'goto' to do so.

This commit restores the original behaviour: for non-refcounted arrays such
as @_ and pad arrays this is not needed, and for pads in particular we may
hit this path many times during the course of a program.

@richardleach I mentioned this difference in #18667, but the focus there was on the ary_offset bug. If you think this requires further discussion I can also open a separate ticket for it.

[richardleach]

This commit restores the original behavious: for non-refcounted arrays such
as @_ and pad arrays this is not needed, and for pads in particular we may
hit this path many times during the course of a program.

The original change was intentional with the thought being that not having to go through the if (av && AvREAL(av)) { branch when creating a new array might help performance on balance. Specifically, my thinking was that creating stacks is comparatively rare, people worried about performance try to avoid unnecessarily creating @_, and modern OO code is likely to create lots of short-lived arrays (which would benefit).

I didn't consider many other kind of non-real arrays at the time, because I don't have a good feel for how common they are and what their creation/growth patterns look like. So me having missed something important of that nature is certainly possible.

However, I did look at pad.c and noted that there's:

• one call to av_extend(): https://github.com/Perl/perl5/blob/blead/pad.c#L2491 - but it looks to pass a real AV which will therefore get Zeroed.
• two calls directly to av_extend_guts() with NULL as the first parameter - https://github.com/Perl/perl5/blob/blead/pad.c#L2571 & https://github.com/Perl/perl5/blob/blead/pad.c#L2624 - so if (av) is not going to be true inside av_extend_guts and the arrays are going to get Zeroed.

So I'm thinking that if pads don't need to be zeroed, the current behaviour in pad.c would have to be changed.

[richardleach]

PS I forgot in the reply above: in pad.c, there are also a number of direct Newx and Newxz calls, including e.g. this one which deliberately avoids calling av_extend(): https://github.com/Perl/perl5/blob/blead/pad.c#L242.

(Other places in core also directly allocate their array and fix up the xpv pointers too. (av_make() does, there's something else that doesn't want the default sizing in another file IIRC. Something that might be nice would be a macro for doing Newx/z & pointer fixup, rather than all these places doing it themselves.)

[richardleach]

Are Perl_pad_push and Perl_padlist_store typically the main functions of concern for run-time performance?

[hvds]

two calls directly to av_extend_guts() with NULL as the first parameter - https://github.com/Perl/perl5/blob/blead/pad.c#L2571 & https://github.com/Perl/perl5/blob/blead/pad.c#L2624 - so if (av) is not going to be true inside av_extend_guts and the arrays are going to get Zeroed

Was that a typo? When if (av) is false, traditionally the arrays would not be zeroed.

Anyway, I tried instrumenting with:

@@ -193,6 +193,13 @@ Perl_av_extend_guts(pTHX_ AV *av, SSize_t key, SSize_t *maxp, SV ***allocp,
              * don't get any special treatment here. */
             ary_offset = 0;
             to_null = *maxp+1;
+            if (av && AvREAL(av)) {
+                ++real_count;
+                real_size += to_null;
+            } else {
+                ++fake_count;
+                fake_size += to_null;
+            }
         }
 
         if (av && AvREAL(av)) {

.. and for lack of a better example of realistic code, ran mktables up to the point of perl_destruct:

% gdb --args ./miniperl -Ilib lib/unicore/mktables -C lib/unicore -P pod -maketest -makelist -p -w
(gdb) break miniperlmain.c:127
(gdb) run
Breakpoint 1, main (...) at miniperlmain.c:127
127         exitstatus = perl_destruct(my_perl);
(gdb) p real_count
$1 = 23501
(gdb) p fake_count
$2 = 57
(gdb) p real_size
$3 = 171209
(gdb) p fake_size
$4 = 436

So colour me convinced - I'll withdraw this PR. Even though the test on i386 is just 4 instructions (including just a 1-byte memory read), it seems clear that performing that test 23558 times to avoid zeroing 436 * sizeof(SV*) bytes is a net loss. If someone has a more realistic workload on which to try the same instrumentation, I'd be interested to see it, but I think I'd expect it to tell essentially the same story.

For future reference: it's really useful if behaviour changes like this are called out by a separate commit, even if that results in a commit that seems trivially small.