Data: avoid using delete on DOM nodes #1664

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants
@jbedard
Contributor

jbedard commented Sep 24, 2014

This is similar to #1662 but for jQuery 1.x which reproduces https://code.google.com/p/chromium/issues/detail?id=378607 in cleanData by using delete on 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):

var div = document.createElement("div");
div.jQuery123 = 1;
*heap snapshot #1*
delete div.jQuery123;
*heap snapshot #2*

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 ] = undefined will just change the expando attribute to jQuery###="undefined" in the DOM, so maybe we need a support test for that and an extra .removeAttribute( internalKey )?

@jbedard

This comment has been minimized.

Show comment
Hide comment
@jbedard

jbedard Sep 24, 2014

Contributor

/cc @markelog, @rwaldron, @gibson042 since you were all interested in the jQuery2 issue

Contributor

jbedard commented Sep 24, 2014

/cc @markelog, @rwaldron, @gibson042 since you were all interested in the jQuery2 issue

test/unit/data.js
- actual = div[ jQuery.expando ];
- }
- equal( actual, expected, message );
+ equal( div[ jQuery.expando ], undefined, message );

This comment has been minimized.

@gibson042

gibson042 Sep 24, 2014

Member

strictEqual

@gibson042

gibson042 Sep 24, 2014

Member

strictEqual

src/manipulation.js
- delete elem[ internalKey ];
-
- } else if ( typeof elem.removeAttribute !== "undefined" ) {
- elem.removeAttribute( internalKey );

This comment has been minimized.

@gibson042

gibson042 Sep 24, 2014

Member

oldIE almost certainly still needs the removeAttribute.

@gibson042

gibson042 Sep 24, 2014

Member

oldIE almost certainly still needs the removeAttribute.

src/manipulation.js
- } else {
- elem[ internalKey ] = null;
- }
+ elem[ internalKey ] = undefined;

This comment has been minimized.

@gibson042

gibson042 Sep 24, 2014

Member

Aside from the post-cleanData expando visibility changes, which I suppose are the central point to debate here, I'd like to see a jsPerf showing the impact.

@gibson042

gibson042 Sep 24, 2014

Member

Aside from the post-cleanData expando visibility changes, which I suppose are the central point to debate here, I'd like to see a jsPerf showing the impact.

This comment has been minimized.

@jbedard

jbedard Sep 24, 2014

Contributor

Here's a jsPerf of the chrome issue: http://jsperf.com/chrome-dom-delete

@jbedard

jbedard Sep 24, 2014

Contributor

Here's a jsPerf of the chrome issue: http://jsperf.com/chrome-dom-delete

This comment has been minimized.

@gibson042

gibson042 Sep 24, 2014

Member

That's too low-level for this discussion... we need to see the impact on methods that call cleanData (e.g., $container.remove()).

@gibson042

gibson042 Sep 24, 2014

Member

That's too low-level for this discussion... we need to see the impact on methods that call cleanData (e.g., $container.remove()).

@jbedard

This comment has been minimized.

Show comment
Hide comment
@jbedard

jbedard Sep 25, 2014

Contributor

I've added the removeAttr back, but the support test is different now since we never actually want to delete.

Here's a jsperf that tests clone+data+remove of 1000 nodes: http://jsperf.com/cleandata/3
And the other jsperf that highlights just the chrome bug: http://jsperf.com/chrome-dom-delete

Contributor

jbedard commented Sep 25, 2014

I've added the removeAttr back, but the support test is different now since we never actually want to delete.

Here's a jsperf that tests clone+data+remove of 1000 nodes: http://jsperf.com/cleandata/3
And the other jsperf that highlights just the chrome bug: http://jsperf.com/chrome-dom-delete

@jbedard

This comment has been minimized.

Show comment
Hide comment
@jbedard

jbedard Sep 25, 2014

Contributor

Or maybe http://jsperf.com/cleandata/4 is more realistic where the .remove is on the parent. Seems about the same though.

Contributor

jbedard commented Sep 25, 2014

Or maybe http://jsperf.com/cleandata/4 is more realistic where the .remove is on the parent. Seems about the same though.

src/manipulation.js
} else {
- elem[ internalKey ] = null;
+ elem[ internalKey ] = undefined;

This comment has been minimized.

@gibson042

gibson042 Sep 26, 2014

Member

I recommend also updating the analogous line in internalRemoveData.

@gibson042

gibson042 Sep 26, 2014

Member

I recommend also updating the analogous line in internalRemoveData.

This comment has been minimized.

@jbedard

jbedard Sep 26, 2014

Contributor

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?

@jbedard

jbedard Sep 26, 2014

Contributor

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?

This comment has been minimized.

@gibson042

gibson042 Sep 26, 2014

Member

Leave the conditions alone; just change null to undefined there as you have done here.

@gibson042

gibson042 Sep 26, 2014

Member

Leave the conditions alone; just change null to undefined there as you have done here.

This comment has been minimized.

@jbedard

jbedard Sep 26, 2014

Contributor

Done, although I still don't understand that deleteExpando check, but I guess that is unrelated.

@jbedard

jbedard Sep 26, 2014

Contributor

Done, although I still don't understand that deleteExpando check, but I guess that is unrelated.

This comment has been minimized.

@gibson042

gibson042 Sep 26, 2014

Member

deleteExpando there is to protect against throwing an exception when the context object is a window.

@gibson042

gibson042 Sep 26, 2014

Member

deleteExpando there is to protect against throwing an exception when the context object is a window.

This comment has been minimized.

@jbedard

jbedard Sep 26, 2014

Contributor

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!

@jbedard

jbedard Sep 26, 2014

Contributor

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!

src/manipulation/support.js
- }
- }
+ div[ jQuery.expando ] = 1;
+ support.expandoAttr = !!div.getAttribute( jQuery.expando );

This comment has been minimized.

@gibson042

gibson042 Sep 26, 2014

Member

Hold onto this for a little bit, because #1652 will give you the expando property for free: gibson042@3da76a3#diff-2555bd940c49b577a6e6d0f57979e80dR39

Also, I'm not sold on the property name. We internally call the phenomena "attroperties"... maybe use that or attrProps (whichever is smaller)?

@gibson042

gibson042 Sep 26, 2014

Member

Hold onto this for a little bit, because #1652 will give you the expando property for free: gibson042@3da76a3#diff-2555bd940c49b577a6e6d0f57979e80dR39

Also, I'm not sold on the property name. We internally call the phenomena "attroperties"... maybe use that or attrProps (whichever is smaller)?

This comment has been minimized.

@jbedard

jbedard Sep 26, 2014

Contributor

I've switched to attrProps. Both attrProps and attroperties (which is an awesome word) actually add 1 byte right now though.

@jbedard

jbedard Sep 26, 2014

Contributor

I've switched to attrProps. Both attrProps and attroperties (which is an awesome word) actually add 1 byte right now though.

src/manipulation.js
@@ -451,17 +451,15 @@ jQuery.extend({
delete cache[ id ];
- // IE does not allow us to delete expando properties from nodes,
- // nor does it have a removeAttribute function on Document nodes;
+ // IE does not allow us to delete expando properties from nodes

This comment has been minimized.

@gibson042

gibson042 Sep 26, 2014

Member

Precede this with // Support: IE<9.

@gibson042

gibson042 Sep 26, 2014

Member

Precede this with // Support: IE<9.

src/manipulation.js
+ // 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)

This comment has been minimized.

@gibson042

gibson042 Sep 26, 2014

Member

Move this comment above the else (with a preceding blank line) and soften its wording to something like "Webkit & Blink performance suffers when deleting properties from DOM nodes, so set to undefined instead". Also move the URL to its own subsequent line to overrun our limits by as little as possible.

@gibson042

gibson042 Sep 26, 2014

Member

Move this comment above the else (with a preceding blank line) and soften its wording to something like "Webkit & Blink performance suffers when deleting properties from DOM nodes, so set to undefined instead". Also move the URL to its own subsequent line to overrun our limits by as little as possible.

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Sep 26, 2014

Member

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 undefined property than take the hit. Does anyone else want to weigh in (@rwaldron, @dmethvin, ...)?

Member

gibson042 commented Sep 26, 2014

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 undefined property than take the hit. Does anyone else want to weigh in (@rwaldron, @dmethvin, ...)?

src/manipulation.js
+ // 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

This comment has been minimized.

@jbedard

jbedard Sep 26, 2014

Contributor

Shall this line remain with the IE comments, or just remove it?

@jbedard

jbedard Sep 26, 2014

Contributor

Shall this line remain with the IE comments, or just remove it?

This comment has been minimized.

@gibson042

gibson042 Sep 26, 2014

Member

It can be removed.

@gibson042

gibson042 Sep 26, 2014

Member

It can be removed.

@jbedard

This comment has been minimized.

Show comment
Hide comment
@jbedard

jbedard Sep 26, 2014

Contributor

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...

Contributor

jbedard commented Sep 26, 2014

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...

Data: avoid using delete on DOM nodes
Ref chromium issue 378607
@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Dec 3, 2014

Member

since this is one of the last PR's to 1.x-master and it seems all issues has been dealt with, i would like to merge it

Member

markelog commented Dec 3, 2014

since this is one of the last PR's to 1.x-master and it seems all issues has been dealt with, i would like to merge it

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Dec 3, 2014

Member

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!

Member

dmethvin commented Dec 3, 2014

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!

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Dec 10, 2014

Member

Closed by 9d1d90e

Member

timmywil commented Dec 10, 2014

Closed by 9d1d90e

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment