The teardown step was calling not only the d3 selector code, but also data( and its argument. This means at teardown we're forcing re-computation of the rendered data. This is expensive, and additionally may not always reliably work (since the UI is in the middle of teardown state is kind of wonky).
Calling data( is not required however, only the d3 selector need to be called.
I confirmed the memory leak fix isn't regressed with this change. For these charts I'm running the test suite and clicking the GC button around ten seconds. The number of nodes steadily increases, but you expect that to happen running QUnit. It doesn't appear skipping data( during teardown changes any behavior.
Before any of the memory leak work you can see listeners barely drop when GC is triggered:
twokul
commented
5 years ago
The teardown step was calling not only the d3 selector code, but also
data(and its argument. This means at teardown we're forcing re-computation of the rendered data. This is expensive, and additionally may not always reliably work (since the UI is in the middle of teardown state is kind of wonky).Calling
data(is not required however, only the d3 selector need to be called.I confirmed the memory leak fix isn't regressed with this change. For these charts I'm running the test suite and clicking the GC button around ten seconds. The number of nodes steadily increases, but you expect that to happen running QUnit. It doesn't appear skipping
data(during teardown changes any behavior.Before any of the memory leak work you can see listeners barely drop when GC is triggered:
Master you can see them drop sharply:
This PR the same: