Data: avoid using delete on DOM nodes

[jbedard]

The jQuery3 data changes started doing delete elem[ expando ] similar to what jQuery1 originally did (see #1664). jQuery2 previously left the data property on the nodes and never deleted it so this wasn't an issue.
#1664 has the originally jQuery1 discussion. Unfortunately jsperf is down right now and the chrome bug seems to be hidden so I'm not sure if it has actually been fixed. But I ran the jsperf a while back and it seemed to still be an issue.

[markelog]

Could you add a comment in the source, of why we don't use delete operator, with references on relevant discussions so we wouldn't forget it next time?

[jbedard]

I've added the same comment that was added to jQuery1 but with an additional link to #1664. The second location references the first instead of copying the same comment.

[markelog]

Hm, i get 403 for this link, you don't?

[jbedard]

I get a 403 as well. It looks like google has made the bug private, maybe due to security issues with the bug?

[markelog]

Hm weird.

@paulirish could you help us out? Can we add another link or something?

[Garbee]

Nothing can really be done here except remove the bug link. From the description of it given here, it either has a known security issue or they are verifying there isn't one before allowing it to be public again.

[markelog]

Right now this is a speculation, there might be a mistake or issues was moved, or basically anything else, i would like to wait day or two for the @paulirish response

[Garbee]

Paul is on vacation. Surely won't respond within a few days.

[markelog]

Thanks! I think we can add Support comment for the Chrome, if we don't have information on the Safari, we can leave something in manner of "Possibly reproducible on Safari too" and i will open a ticket for it, so we could deal with it later, when jsperf would be up.

Duplicate that comment in manipulation module too and since this matter was previously discussed already we can safely land it.

Sounds good?

[arthurvr]

👍

[markelog]

We need to verify this bug is still relevant, since jsperf is down, which is pretty unfortunate, would you mind putting together a repo with perf test of benchmarkjs?

[mgol]

@jbedard You can see in #2439 (comment) how I did that for fractional .css('width') etc.

[jbedard]

Thanks @mzgol I'll give that a shot

[mgol]

I landed #2480 so this needs to be rebased on latest master.

Note that since I landed 5fe76c6 on master & backported the tests in 624d6a8580a7e1108c81a6a777dc1be3d408bddf on compat, there is a slight difference in these test - on master we're checking if the expando property is removed, on compat we check if its value is set to undefined. It would be good to converge both branches on the same behavior, this PR is a good way to do it.

@jbedard could you create an issue, similarly to how you created issue #2503 for PR #2480?

[mgol]

I'd like to land #2526 first as this will change a few things in this PR.

[jbedard]

Sorry for the delay.

I've rebased this. Want me to make the tests more like 624d6a8 though? (Currently they assert dom[key] == undefined where 624d6a8 avoids the assert if undefined...).

Here is the perf test I've been running (and other similar tests):

<script src="https://code.jquery.com/jquery-git.js"></script>
<script>
    var $j = jQuery.noConflict();
</script>
<script src="../dist/jquery.js"></script>
<script src="https://cdnjs.cloudflare.com/ajax/libs/benchmark/1.0.0/benchmark.js"></script>
<script>
var TEMPL = document.createElement('div');
for (var i=0; i<100; i++) {
    TEMPL.appendChild(document.createElement('div'));
}
function test($) {
    var n = TEMPL.cloneNode(true);
    document.body.appendChild(n);
    for (var i=0, ii=n.childNodes.length; i<ii; i++) {
        $.data(n.childNodes[i], "foo", i);
    }
    $(n).remove();
}

new Benchmark.Suite()
    .add('DOM delete', function() {
        test($j);
    })
    .add('DOM assign', function() {
        test($);
    })
    .on('complete', function() {
        console.log('Fastest is ' + this.filter('fastest').pluck('name'));
    })
    .run();
</script>

Generally I'm finding the assign case 5-10% faster on Chrome43. This is nothing compared to the original issue with ~Chrome35 where I think it was 500%. The other measurement was memory usage, doing node = document.createElement("div"); node.property = 42; delete node.property currently increases the memory usage by ~400 bytes compared to replacing the last statement with node.property = undefined. Again this is nothing compared to ~Chrome35 which increased it by >6k (and lead to major GC issues sometimes crashing chrome).

[jbedard]

Do we want the full comment copied here and below? Seemed like a lot to put in 3 places...

[jbedard]

Looks like jsperf is back up and the original test (https://jsperf.com/cleandata/4) shows similar results to my snippet above (delete is still slow on chrome). Would be great if someone could test with the latest safari (or maybe the jsperf already has data from the latest safari, but the data isn't loading for me right now...).

[jbedard]

I've created #2530 for this.

[markelog]

delete is still slow on chrome

Hm, could you move

var p = jQuery(testDOM.cloneNode(true));
p.children().data("test", 123);

bit, out of the test run? Also, could you compare these changes with 3.0 alpha?

[jbedard]

We need new nodes for each test run though since the chrome bug only occurs the first time you delete from a node (I think?). Normally you only .remove() a node once anyway. Is there a way of recreating the nodes before each run? If I understand correctly the setup is only run once "before each clocked test loop" not before each individual test run?

[markelog]

Check it out - http://benchmarkjs.com/docs#prototype_setup

[jbedard]

That confirms what I was saying. The setup executes once outside the loop where we want a new node once per loop...

[markelog]

Yeah, it does, it also means you can move those instructions out side of the test run, just like it shown in the link i posted

[jbedard]

So create N nodes outside the loop and .remove() one per loop? That would work...

I'll try to update the jsperf and compare master with vs without this PR, hopefully later today.

[markelog]

Thank you :-)

[paulirish]

btw this is the response from V8 on the ticket:

dcarney@

This is all as expected.

Adding a property with the same name to all elements of the same type creates one class to describe that new object.

Adding a differently named property will create a class for each element. Classes are not tiny.

Deleting a property will transition it into a dictionary object that it both larger and unoptimized in many cases. It is to be avoided on most if not all engines.

[jbedard]

@markelog I've updated the jsperf to compare this PR vs 3.0 and to only measure $(arrayOfNodes.pop()).remove(): https://jsperf.com/cleandata/5

@paulirish thanks for the info! That makes me wonder if jquery should also go back to having a single expando per element instead of two...

[markelog]

@jbedard now we talking, 40%, not some 12%, this is probably not a bottleneck, but worth a few bytes.

@paulirish thank you for the info, now we know exactly why v8 is slower from monomorphic point of view that is, jk of course, nevertheless, ff gives same ops/sec for both cases, so i would suggest to implement additional optimizations for this specific case.

That makes me wonder if jquery should also go back to having a single expando per element instead of two...

I feel that sometimes the more we know about internals of js engines, the more it provokes devs to create a weirder code-paths.

Okay, so, given that perf optimization are now could be considered as proven, whereas byte size impact is minimal and that we already do this for the compat, i'd say it safe to land with one more additional review.

Will ping @timmywil regardless.

[gibson042]

No need to reference the other PR; followup will start with blame and/or log anyway.

[gibson042]

Other than an extra comment line, looks good to me.

[jbedard]

Removed the extra comment line...

[jbedard]

I've added one additional change: now that we assign undefined instead of using delete the non-node defineProperty call doesn't need configurable: true and only needs the writable: true. Also updated the comment explaining this.

[timmywil]

So, in compat, we delete the cache property for anything non-node. While this PR aligns behavior for nodes, it changes behavior for non-nodes. I would like to have the same behavior on both branches.

[jbedard]

Updated to align with compat more. I think that means we only need configurable: true and not writable: true although that part can be skipped if we want to keep this change to only what is required. Also added a test for this non-node case.

[timmywil]

Looking good! Thanks @jbedard!

[mgol]

@jbedard I rebased your commit over latest master (there were major changes in how we use the QUnit interface) and uploaded it as a branch pr/2479; the latest commit is this rebased one.

I get one event failure, in the test ".off() removes the expando when there's no more data", see the code here:

jquery/test/unit/event.js

Lines 2698 to 2717 in c9cf250

	QUnit.test( ".off() removes the expando when there's no more data", function( assert ) {
	assert.expect( 1 );
	var key,
	div = jQuery( "<div/>" ).appendTo( "#qunit-fixture" );
	div.on( "click", false );
	div.on( "custom", function() {
	assert.ok( true, "Custom event triggered" );
	} );
	div.trigger( "custom" );
	div.off( "click custom" );
	// Make sure the expando is gone
	for ( key in div[ 0 ] ) {
	if ( /^jQuery/.test( key ) ) {
	assert.ok( false, "Expando was not removed when there was no more data" );
	}
	}
	} );

Would you like to have a look? You can hard-reset your branch to the one I created and work from there.

[jbedard]

I assume that test needs to be updated to do strictEquals( div[ 0 ], key, undefined, ...) now that the expando is set to undefined instead of being deleted. I'll try to update the PR later today...

[mgol]

@jbedard Ah, you're right. Do you want to do it or should I modify the PR & land?

[jbedard]

You can go ahead if you want, otherwise it will be another 8h or so before I get around to it...

[mgol]

OK, landed at 0e98243, thanks! I also backported the test to compat at 5a7674d.

[jbedard]

Awesome, thanks!