New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Suspected memory leak event handler assignment with data binding through key function #186
Comments
If you do not retain any references to the removed elements, it shouldn’t matter that they still have event listeners bound to them; the browser garage collector will be able to dispose of them. (This type of circular reference used to be a bug years ago in some browsers, but this is not the case in modern browsers.) |
I made a little test case but I’m not able to reproduce this reliably, and it also seems to be affected by browser extensions and their MutationObservers… At any rate, I don’t think it’s appropriate for selection.remove to remove all registered event listeners silently, since you may intended to add the selected elements back to the DOM in the future. I think it would be possible to use a WeakMap keyed by element rather than using element.__on to stash the event listener, though. If you want to try that can let me know if it helps, I’d be willing to merge a PR. |
Thank you Mike, I am happy with reassigning the event handlers, for my situation not a real performance issue. To reproduce, I think it boils down to the following: elements = svg.selectAll('.whatever').data(arr, keyfunction).... enterelements = elements.enter() enterelements.merge(elements).on('click', ...) <- this solves the problem for cloned datasets |
Ah ha! The leak, then, is because listeners capture their associated group (an array of elements from the selection). If you do a data join and you don’t reassign a listener to the updating elements, the old listeners retain a reference to the old group prior to the join, including now-removed elements. Whereas if you reassign a listener to the merged enter + update, then you dispose all references to your old elements. The simplest thing to do would be to avoid capturing the group, but that wouldn’t be backwards-compatible, and it would make the listener callback signature different from the standard D3 pattern. (The intent of the design was to make them consistent.) An element can exist in multiple selections simultaneously, so we don’t want to persist the index and group on the element, either, like we do with bound data. So I don’t think we can silently change the group bound to the old listeners, too. |
For your information, I noticed similar behavior for cloned datasets in string interpolation of the transform attribute. If I create a transition with a string interpolation of translate(x, y) via an each loop, the last element is still referenced and when removed it becomes detached with a reference in the d3 scope. .transition().duration(..) .transition().duration(..) |
When using a key function to join data and "immutable" data structures, which completely renew the whole dataset after some modification, D3 assigned event handlers keep a reference to the old data in the __on property.
When removing elements via exit(), detached SVG elements remain in memory by the scope present in __on, causing a memory leak. Although these leaks can be prevented by reassigning the event handlers for existing elements as well (e.g. after merge) it would be more convenient and less error prone, when the only data binding would be through selection.data.
The text was updated successfully, but these errors were encountered: