Memory leak fixes

[monfera]

Commit c123ebc helps a lot - not only with a significantly reduced (though not yet fully eliminated) memory leak, but also, causes a stop for the point picker requestAnimationFrame loop, so the purportedly idempotent plotNew function doesn't leave behind an orphaned, CPU eating rAF loop with potentially large arrays.

It may be that part of the residual leakage is related to the rAF acting as a closure with variable capture: even if the rAF is stopped by causing stop = true via the intended destroy sequence, maybe some state is still retained. Check this among other things.

Unrelated to the leakage, we identified the rAF based approach as something we might want to improve upon.

PR is WIP not only because it's not a full fix and it's as yet untested, but also the code will probably need to be more generic (e.g. now it shoots for _scene2d but the 3D case may be analogous).

[etpinard]

Hmm. Gl context deletion should be already taken care of via Plots.cleanPlot which is currently called in Plots.supplyDefaults here and more explicity in Plotly.purge here.

Maybe something is up with the gl2d clean method?

[monfera]

@etpinard I was chasing a point pick regression but back on this now. The root call is a Plotly.newPlot. It does call supplyDefaults and cleanPlot. But basePlotModules is [] empty so it doesn't even reach the point of testing if there is a _module.clean metod. I'm carrying on looking, based on your hint, but wanted to mention it in case it rings a bell.

P.S. In fact, oldFullLayout looks like an empty object. I'm swimming upstream.

[monfera]

... looks like plots.purge deletes gd._fullLayout just before Plotly.plot is called (the latter of which then calls supplyDefaults which calls cleanPlot) so by the time cleanPlot is called, there's no longer information about what to clean.

I'm in a bit of a conundrum as there's a big amount of statefulness and I'm concerned that a purported fix from me (e.g. not delete gd._fullLayout in purge so the info stays around) has unintended, undesirable consequences, besides the fact that it would allow proper working of the _module.clean loop. So I'd rather get input @etpinard before shoehorning in something that looks like a fix.

Plotly.newPlot = function(gd, data, layout, config) {
    gd = getGraphDiv(gd);
    Plots.purge(gd);   // <= this deletes gd._fullLayout
    return Plotly.plot(gd, data, layout, config);  // <= cleanPlot would rely on gd._fullLayout
};

[etpinard]

so by the time cleanPlot is called, there's no longer information about what to clean.

Wow. That's bad. Nice catch.

I think the best solution at the moment would be to add

 // remove gl contexts
Plots.cleanPlot([], {}, fullData, fullLayout);

to Plotly.newPlot before it calls Plots.purge.

[monfera]

Thanks @etpinard! This

Plotly.newPlot = function(gd, data, layout, config) {
    gd = getGraphDiv(gd);

    // remove gl contexts
    Plots.cleanPlot([], {}, gd._fullData || {}, gd._fullLayout || {});

    Plots.purge(gd);
    return Plotly.plot(gd, data, layout, config);
};

seems to go ahead and ultimately call scene2d destroy method which has my few lines of trace line disposals. I'll double-check via remeasuring the memory footprint, but for that test I have to cherrypick it into the local branch which also has the undo history limit.

[etpinard]

@monfera thanks for looking into this!

[monfera]

@etpinard I pushed the change you suggested, and the pick fix (just brought forward the trace dispose loop), so I think it's ready for review.

[etpinard]

Great!

@monfera would you adding a test case for checking that Plotly.newPlot does clear existing gl contexts similar to the one for Plots.cleanPlot ?

Thanks in advance.

[etpinard]

@monfera Looks good!

Thanks again for catching that newPlot bug.

[etpinard]

Great tests! Thanks @monfera !!