TONYC TPF Grant 6 report #16

[Hours]         [Activity]
2016/02/08      Monday
 1.02           #127474 review, testing, apply to blead
 0.37           #127384 review, research and comment
 1.32           #126410/#124387 testing, minor optimization, apply to
                blead
=====
 2.71

2016/02/09      Tuesday
 0.80           #127386 comment with new patch
 0.25           testing rjbs' Time::HiRes move, method return value
                refcounting discussion on #p5p
 0.13           #127386 comment some more
 0.57           #125833 update patch
 1.20           #125351 debugging
 2.50           #125351 debugging
=====
 5.45

2016/02/10      Wednesday
 3.45           #127494 debug, find cause and work on a patch, testing
 0.63           #127494 re-check, testing, apply to blead and comment
 0.70           #127334 produce a fix and apply to blead
 0.13           #127260 comment
 0.45           #126871 try to reproduce
=====
 5.36

2016/02/11      Thursday
 1.60           #127508 testing (after dealing with MS's breakage of my
                git installation), apply to blead
 0.23           BBC reviews (122136 amongst them)
 0.70           #127231 testing, comment
 0.62           #126472 review and summarize cpan fixes
 0.68           #127511 look over new solaris thread-dirh.t test failures,
                open ticket, email discussion with jhi
 0.33           solaris threaded/dtrace/debugging build failure, try to
                find breaking commit by inspecting, start bisect
 0.88           cygwin Win32API-File/t/file.t test failure
=====
 5.04

Which I calculate is 18.56 hours.

Approximately 14 tickets were reviewed or worked on, and 5 patches
were applied.

[perl #126410] and [perl #124387] are related in that both problems
are related to the same set of changes.

Between perl 5.8 and 5.16 inclusive, perl cached the CV for DESTROY in
overload magic for the stash.  This was four times faster than the
simple method lookup previously done each time an object needed to be
destroyed.

This was much faster for destruction, but meant that overload magic
was created for any stash that objects were destroyed for, taking up
576 bytes (on x64) for a stash with a DESTROY method, even if it did
no overloading.

In 5.18 this mechanism was changed to store the DESTROY CV in the
stash's stash slot, and stored (CV*)1 if there was no DESTROY method.

To ensure that a stale CV wasn't used if the DESTROY method was
changed, it kept the current PL_sub_generation in the stash's MRO
metadata, and checked it on each call to DESTROY.

This caused two problems:

[perl #126410] Code that checked the stash slot unexpectedly found
(CV*)1 in the slot, which is non-NULL, and so attemptted to treat it
as a hash.  Which crashed:

 $ perl -Mversion -MDevel::Peek -e 'version->new("1.0"); Dump(\%version::)'
 SV = IV(0x1db4e68) at 0x1db4e78
   REFCNT = 1
   FLAGS = (TEMP,ROK)
   RV = 0x1db5388
   SV = PVHV(0x1dbb8f0) at 0x1db5388
     REFCNT = 3
     FLAGS = (OOK,OVERLOAD,SHAREKEYS)
     STASH = 0x18Segmentation fault

[perl #124387] The change inadvertently didn't handle AUTOLOAD for the
DESTROY method.

To fix this, instead of storing the DESTROY CV in the stash slot I
kept it in the stash's MRO metadata, alongside the sub generation.

A second patch added AUTOLOAD support, which I managed to do
incorrectly, which caused CPAN failured, reported as [perl #127494].

The problem was my code called gv_fetchmethod_pvn_flags() with the
GV_AUTOLOAD flag, and whether that was actually and AUTOLOAD or
DESTROY method, cached the CV.

When looking for an AUTOLOAD method the various APIs set $AUTOLOAD to
the expected method name[1] so the AUTOLOAD method can load, call or
emulate the correct method.

By caching the AUTOLOAD method, that initialization wasn't done, so
AUTOLOAD would see the name of the previous method that was autoloaded
instead of DESTROY.

To fix this, I only cache the DESTROY CV if it's from DESTROY, not if
it's AUTOLOAD.

It might have been a little faster to store whether the CV stored was
DESTROY or AUTOLOAD, and perform the required setup when calling
AUTOLOAD, but it didn't seem worthwhile - I expect the processing done
by AUTOLOAD itself to significantly drown that out.

[1] it works a bit differently if AUTOLOAD is XS

• TONYC TPF Grant 6 report #16 by Tony Cook
• Re: TONYC TPF Grant 6 report #16 by Achim Gratz
• Re: TONYC TPF Grant 6 report #16 by Tony Cook
• Re: TONYC TPF Grant 6 report #16 by Achim Gratz