Bug #132332 for Object-Pad: Memory leak when accessing pads

Sun Apr 12 17:56:04 2020 TEAM [...] cpan.org - Ticket created

Subject:

Memory leak when accessing pads

The attached test case shows a leak (tested on perl 5.30 with Object::Pad 0.20, am guessing that all Perl versions are affected).

Subject:

object-pad-leak.t

use strict; use warnings; use Test::More; use Test::MemoryGrowth; use Object::Pad; class Example { # Needs at least one field member to trigger failures has $thing; # ... and we need to refer to it in a method as well method BUILD { $thing } } no_growth { Example->new }; done_testing;

Mon Apr 13 13:11:39 2020 TEAM [...] cpan.org - Correspondence added

Some observations so far: - Devel::MAT shows a long list of empty AVs from the test case - each of these has refcnt 1, but no inrefs nor outrefs - at a guess, this *could* be the meta->slots AV... - ... and it's not immediately clear what part of the code would free this? There's a newAV but no obvious mechanism for reducing the refcnt on destruction or av_undef()-ing it out of existence?

Tue Apr 14 10:29:01 2020 leonerd-cpan [...] leonerd.org.uk - Correspondence added

Confirmed repeatable: $ prove -vb object-pad-leak.t object-pad-leak.t .. not ok 1 # Failed test at object-pad-leak.t line 15. # Lost 540672 bytes of memory over 10000 calls, average of 54.07 per call # Writing heap dump to object-pad-leak-1.pmat # Writing heap dump after one more iteration to object-pad-leak-1-after.pmat 1..1 # Looks like you failed 1 test of 1. ... leo@shy:~/src/perl/Object-Pad [bzr] $ pmat-diff object-pad-leak-1{,-after}.pmat A B ARRAY(0) at 0x55d8202745e8 HASH(0) at 0x55d820274690 UNDEF() at 0x55d8202748e8 HASH(0) at 0x55d820274948 UNDEF() at 0x55d820274960 --- 2 unique to A 3 unique to B 33679 common On Mon Apr 13 13:11:39 2020, TEAM wrote: Show quoted text

> Some observations so far: > > - Devel::MAT shows a long list of empty AVs from the test case > - each of these has refcnt 1, but no inrefs nor outrefs > - at a guess, this *could* be the meta->slots AV... > - ... and it's not immediately clear what part of the code would free > this? There's a newAV but no obvious mechanism for reducing the refcnt > on destruction or av_undef()-ing it out of existence?

It does indeed: $ pmat object-pad-leak-1-after.pmat Perl memory dumpfile from perl 5.30.0 threaded Heap contains 33682 objects Show quoted text

pmat> show 0x55d8202745e8

ARRAY(0) at 0x55d8202745e8 with refcount 1 size 64 bytes 0 elements (use 'elems' command to show) Show quoted text

pmat> identify 0x55d8202745e8

ARRAY(0) at 0x55d8202745e8 is: └─the lexical @(Object::Pad/slots) at depth 1 of CODE(PP) at 0x55d820112290, which is: └─the symbol '&Example::BUILD' A new empty array. Note this isn't the backing array of any instance of the class. Also it has zero elements in it, whereas an instance would have an element. I wonder if this is a new empty array created by the saved "clear pad index" operation. Possibly a different mechanism is required here; as the array is just an alias. -- Paul Evans

Tue Apr 14 10:29:02 2020 The RT System itself - Status changed from 'new' to 'open'

Tue Apr 14 10:53:55 2020 leonerd-cpan [...] leonerd.org.uk - Correspondence added

On Tue Apr 14 10:29:01 2020, PEVANS wrote: Show quoted text

> A new empty array. Note this isn't the backing array of any instance > of the class. Also it has zero elements in it, whereas an instance > would have an element. > > I wonder if this is a new empty array created by the saved "clear pad > index" operation. Possibly a different mechanism is required here; as > the array is just an alias.

Yes; it appears to be the result of the unwind code for SAVEt_CLEARSV: if (SvREFCNT(sv) == 1 && !SvOBJECT(sv)) { ... (won't apply here) } else { switch (SvTYPE(sv)) { case SVt_PVAV: *svp = MUTABLE_SV(newAV()); break; ... -- Paul Evans

Tue Apr 14 11:08:40 2020 leonerd-cpan [...] leonerd.org.uk - Correspondence added

Possible solutions: 1) Allow the newAV to sit after method returns, but throw it away next time we're invoked: save_clearsv(&PAD_SVl(PADIX_SLOTS)); + /* The previous time perl unwound a save_clearsv() it stored a newAV() into + * the pad. We'd better destroy that first (RT132332) + */ + if(PAD_SVl(PADIX_SLOTS) && PAD_SVl(PADIX_SLOTS) != &PL_sv_undef) + SvREFCNT_dec(PAD_SVl(PADIX_SLOTS)); PAD_SVl(PADIX_SLOTS) = SvREFCNT_inc(slotsav); return PL_op->op_next; A downside here is a small (but asymptotically constant) growth in memory requirements as eventually every `method` CV in the program will have one of these dead AVs sitting with it, never to be used. It'll get worse with recursion if multiple pad depths are used. Associated with this is the additional CPU time overhead involved in repeatedly allocating these never-used AVs just to throw them away next time. 2) Don't use save_clearsv() but instead use a combination of save_freesv() + SAVESPTR() to defer an SvREFCNT_dec() and set the pad SV back to NULL. - save_clearsv(&PAD_SVl(PADIX_SLOTS)); + SAVESPTR(PAD_SVl(PADIX_SLOTS)); PAD_SVl(PADIX_SLOTS) = SvREFCNT_inc(slotsav); + save_freesv(slotsav); return PL_op->op_next; This has no runtime overhead as mentioned with option 1, but has a downside that currently the savestack suspend/resume logic in Future::AsyncAwait does not understand this structure, so all the `async method` tests fail: t/80async-method.t .............. 1/? Future::AsyncAwait panic: TODO: Unsure how to handle savestack entry of 41 For reference, #define SAVEt_SPTR 41 That could be fixed by a new version of F-AA which can at least specialcase handle a SAVEt_SPTR whose target variable is a pad slot - a test that can be performed efficiently by pointer range comparison (the SV ** just has to be between AvARRAY(curpad) and AvARRAY(curpad)+AvMAX(curpad); so no linear scan required). -- Paul Evans

Tue Apr 14 19:30:28 2020 leonerd-cpan [...] leonerd.org.uk - Correspondence added

On Tue Apr 14 11:08:40 2020, PEVANS wrote: Show quoted text

> 2) Don't use save_clearsv() but instead use a combination of > save_freesv() + SAVESPTR() to defer an SvREFCNT_dec() and set the pad > SV back to NULL.

I've gone for this option, and fixed Future::AsyncAwait ready to cope with it. There'll be a corresponding new release of that dist as well. Additionally, the same problem will occur with any array or hash slots, so those need fixing too. -- Paul Evans

Wed Apr 15 07:34:52 2020 leonerd-cpan [...] leonerd.org.uk - Status changed from 'open' to 'resolved'

Wed Apr 15 07:34:53 2020 leonerd-cpan [...] leonerd.org.uk - Fixed in 0.21 added