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]

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 https://github.com/jquery/jquery/pull/2439#issuecomment-118898816 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 5fe76c663f8a4986af62edb434a1708c006d0b21 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 624d6a8580a7e1108c81a6a777dc1be3d408bddf though? (Currently they assert dom[key] == undefined where 624d6a8580a7e1108c81a6a777dc1be3d408bddf 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]

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]

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: https://github.com/jquery/jquery/blob/c9cf250daafe806818da1dd207a88a8e94a4ad16/test/unit/event.js#L2698-L2717

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!