Data: avoid using delete on DOM nodes #1664
Conversation
|
/cc @markelog, @rwaldron, @gibson042 since you were all interested in the jQuery2 issue |
| actual = div[ jQuery.expando ]; | ||
| } | ||
| equal( actual, expected, message ); | ||
| equal( div[ jQuery.expando ], undefined, message ); |
|
I've added the Here's a jsperf that tests clone+data+remove of 1000 nodes: http://jsperf.com/cleandata/3 |
|
Or maybe http://jsperf.com/cleandata/4 is more realistic where the |
| } else { | ||
| elem[ internalKey ] = null; | ||
| elem[ internalKey ] = undefined; |
There was a problem hiding this comment.
I recommend also updating the analogous line in internalRemoveData.
There was a problem hiding this comment.
In internalRemoveData it does...
if ( isNode ) => cleanData( ... )
else if ( support.deleteExpando || !isWindow ) => delete
else => assign null
Doesn't support.deleteExpando mean that it is safe to delete from nodes? But in this case we already know it is not a node... Or does support.deleteExpando also test for more general non-node delete issues?
There was a problem hiding this comment.
Leave the conditions alone; just change null to undefined there as you have done here.
There was a problem hiding this comment.
Done, although I still don't understand that deleteExpando check, but I guess that is unrelated.
There was a problem hiding this comment.
deleteExpando there is to protect against throwing an exception when the context object is a window.
There was a problem hiding this comment.
Ah, I see, and the deleteExpando test on a div tests for the same issue as deleting from the window (and leaking a test var onto the window would be lame...). Thanks!
|
http://jsperf.com/cleandata/4 is exactly what I was looking for, and clearly shows the performance impact on Webkit and especially Blink. Given their severity, I'd rather leave an |
| // IE does not allow us to delete expando properties from nodes | ||
| // IE creates expando attributes along with the property | ||
| // IE does not have a removeAttribute function on Document nodes | ||
| // Chrome has issues deleting from nodes (https://code.google.com/p/chromium/issues/detail?id=378607) | ||
| // we must handle all of these cases |
There was a problem hiding this comment.
Shall this line remain with the IE comments, or just remove it?
|
I've made the changes, let me know if I missed anything. Just need to add the new attrProp to the support tests which should be pushed in a few mins... |
Ref chromium issue 378607
|
since this is one of the last PR's to |
|
Yes, let's merge this. It looks like the underlying Chrome bug has been there a while so there is no guarantee they will fix it soon. This patch mentions the bug so we can review and change it in the future if it ever gets fixed. Thanks @jbedard! |
|
Closed by 9d1d90e |
This is similar to #1662 but for jQuery 1.x which reproduces https://code.google.com/p/chromium/issues/detail?id=378607 in
cleanDataby usingdeleteon DOM nodes. I have found this issue worse then the jQuery2 version because cleanData often runs on large groups of nodes at once (such as when changing pages in a SPA). If every node (with data) reproduces the chrome issue it can cause memory usage to spike very high and cause significant performance issues. I have seen pages spike from 15m to 150m while transitioning pages in a SPA, combined with https://code.google.com/p/v8/issues/detail?id=2073 that use case often crashes the browser.The easiest way to reproduce it, in the chrome console (on any page, a blank one makes the memory snapshot easier to read):
If you find the detached div element in the heap snapshot it is about 150 bytes in snapshot 1, in snapshot 2 I'm normally seeing it around 6300 bytes.
I'm having trouble testing this properly in oldIE so I'm not 100% sure if the current commit covers oldIE correctly. I think setting
elem[ $.expando ] = undefinedwill just change the expando attribute tojQuery###="undefined"in the DOM, so maybe we need a support test for that and an extra.removeAttribute( internalKey )?