Memory leak(s) and PR to fix them

[jezwhite]

Hello all,

I have come across various minor memory leaks and a major one when using closures. It's been a while since I've used XS and there are things I don't understand so I wanted to run through some of my proposed changes.

Minor leaks - Changes

The first change would be to add a dedicated destructor, something like:

static void destroy_duktape_object(pTHX_ Duk* duk) { 
  if (duk) {   
    //Free perl objects 
    SvREFCNT_dec((SV *) duk->stats);
    SvREFCNT_dec((SV *) duk->msgs);
    free(duk);
  }
}

Which would be called in session_dtor. However, I don't understand the line:

static MGVTBL session_magic_vtbl = { .svt_free = session_dtor };

and why all associated magic logic in the typemap? Why isn't a simple destructor used? ie,

void DESTROY (Duk* duk)
PPCODE:
  destroy_duktape_object(aTHX_ duk);

There are a couple of other minor leaks, such as reset_msgs and reset_stats but these are simple to fix.

Major Leak - Closures

This is causing me massive problems. The following (contrived) example will highlight the problem when using closures:

use warnings;
use strict;
use JavaScript::Duktape::XS;

my $options = {
    gather_stats     => 1,
    save_messages    => 1,
    max_memory_bytes => 256*1024,
    max_timeout_us   => 2*1_000_000,
};

for (1..10000) {
   #Create a new VM everytime as we need it to be 'safe' and 'clean'
  my $vm = JavaScript::Duktape::XS->new($options);
  #Create a silly large object that we want to expose to the JS environment
  my $largeObject = {};
  for (1..1000) {
    $largeObject->{$_}= 'a large object';
  }

  #Add a perl function to the vm, creating a closure
  $vm->set('SomeTask', sub {
      #extract from our object and return it to the JS environment
      return $largeObject->{1};
  });
  #Before the container is destroyed, we remove the function hoping that
  #the ref count to the closure is dropped but it doesn't...
  $vm->remove('SomeTask');
}

I believe the problem is in pl_perl_to_duk_impl, and this line:

func = newSVsv(value);

This newly created SV is never freed, thus the closure can never be either. I think doing a SvREFCNT_inc rather than newSVsv on the code ref SV and dropping it again when the coderef isn't needed solves the problem (it seams to in my basic testing).  The change would need to keep track of the coderef and do the appropriate SvREFCNT_dec when the code ref is replaced (->set ). removed (->remove) and when the main duktape object is destroyed. 

Thoughts? Comments?

[gonzus]

Hello @jezwhite thanks for your questions and comments.

I have not used this module in quite some time, so my memory is vague and I may not have (good) answers for some / all of your questions.

In general, I will give preference to any PR that includes a test case which fails before the PR (showing the error condition) and which succeeds after the PR.

Regarding your specific questions:

1. I don't really remember anymore the reason for the magic which ends up calling the destructor. There was a reason for it, but it is lost in my memory. I would rather you add the destroy_duktape_object you propose, and a call to it from the current destructor.
2. Your idea to deal with the leak associated with func makes sense, but perhaps the difficulty is knowing when you can free it safely? If this is hard / impossible to determine, maybe exposing that functionality would be enough, giving the Perl code an opportunity to clean things up.

Looking forward to any PRs. Cheers!

[jezwhite]

In general, I will give preference to any PR that includes a test case which fails before the PR (showing the error condition) and which succeeds after the PR.

Understood. So in this case, it's pure memory leaks. Can you suggest any Test module I can use to check memory usage before call(s) and afterwards that you would be happy for me to use? Or just something simple like https://metacpan.org/pod/Memory::Usage ?

2. Your idea to deal with the leak associated with func makes sense, but perhaps the difficulty is knowing when you can free it safely? If this is hard / impossible to determine, maybe exposing that functionality would be enough, giving the Perl code an opportunity to clean things up.

So...yeah, it was harder than I thought once I started looking at the internals:) I had to go simple but I think I have something that is working and passes all tests, but there is an edge case I am unsure of so I still testing/playing.

Looking forward to any PRs. Cheers!

I will put one together - would you mind giving it look over to see if seems 'ok'?

[gonzus]

I have used Test::LeakTrace to great success in the past. I think it only works on Linux, though.

Happy to review any proposed PR.

Cheers!