Thanks for contributing! There is an existing patch that will change how data is created and managed #1428

I thought @gibson042 wanted this in?

#1428 will be great, but will it make it into the next release? And will this change (avoiding defineProperty) make it into #1428?

Sorry, I was mobile when I was looking at this and looked right past that part.

@jbedard would you mind post here a resulting jsperf? With and without your changes that is?

Here's an old one just comparing assignment/defineProperties/defineProperty: http://jsperf.com/v1-v2-data

Here's one testing clone+data+remove: http://jsperf.com/remove-dp and without the remove: http://jsperf.com/remove-dp/2

This is using the latest build of master with vs without this change.

Here's an old one just comparing assignment/defineProperties/defineProperty: http://jsperf.com/v1-v2-data

This isn't an accurate representation of the operations that jQuery 1 and 2 are doing for data. That op happens only once per object for any object that store arbitrary data.

http://jsperf.com/remove-dp is attempting to remove nodes that were never attached, which will affect the result. http://jsperf.com/remove-dp/2 is closer, but also measuring the cost of jQuery(elem.cloneNode(true))

I was working on a revision but got blocked by jsperf's spam filter mechanism.

But that is what we want to measure, only the first data operation that happens once per object, that is the only thing improved by this change.

The clone/remove was mainly to show that this change has an effect on overall performance when performing a standard operation such as constructing a DOM and adding some data (although .append is probably better then.remove after that...). Just showing that .data is faster then before (which http://jsperf.com/v1-v2-data sort of does) doesn't really put it into perspective.

I've hit that jsperf spam issue as well recently :(

Comparing the measurement of current and patched .data("key", value) for a large number of elements should be sufficient. This is what I was putting together when I got blocked :\

@jbedard or @rwaldron do you want to do a jsperf for this?

Did you want something other then the jsperfs mentioned above? I think those show the advantage of this change quite well...

@dmethvin landing this will have shave some time off, but nothing like the perfs shown above.

http://jsperf.com/jquery-setting-initial-data

(compares 1, 2, and this patch)

If you only want to measure the first call to .data, nothing else, then I think http://jsperf.com/jquery-setting-initial-data/2 is a bit better (avoids .children). It might also be good to compare against the latest master instead of 2.1 to avoid the recently fixed chrome perf issue: http://jsperf.com/jquery-setting-initial-data/3

Seems well worth it to me. It improves in every browser, in every jsperf, and reduces size...

@dmethvin I think this is reasonable to land.

Awesome, thanks for the work on this everyone!

I'll pile on bad news, at the moment it looks like ES6 may actually be making Object.defineProperties and Object.create slower by following the lead of Object.assign to hold on to the first error thrown and continue executing which may hurt perf in much the same way as try-catch and complicate inlining.

@jdalton this is not the appropriate place to report ES6 specification issues. Please file here: https://bugs.ecmascript.org/enter_bug.cgi?product=Draft%20for%206th%20Edition

Yap, I'm aware of where to file spec bugs. Just giving a heads up that the perf forecast for these methods at the moment isn't getting better.

But if you file a bug, the issue will at least get attention before ES6 ships. You should also loop in @bterlson on spec-responsible performance issues.

@rwaldron: not to worry, we're on it (though no promises there will be any change to the current draft ofc)

@bterlson is it worth bringing up at the next meeting?

@rwaldron Yep, worth discussing.

https://github.com/tc39/agendas/blob/master/2014/11.md

If the new exception handling in Object.{assign, create, defineProperties, freeze, isFrozen, isSealed, seal} is considered to be too harmful for performance, exception behaviour in for-in and for-of most likely also needs to be changed.

@anba yeah, good to discuss those too. I don't think for-in/for-of semantics will change since their behavior is well motivated (and long-discussed), but the Object API changes seem less well motivated.

Rock y'all, the issue raised earlier has been resolved by the TC39.

I am inclined to land this as-is for 3.0 but want to note here that with gh-1886 and deprecating plain objects we should be able to always use the expando approach at some point. Thoughts?

We have always taken pains to prevent the property required by .data from causing problems... even the compat branch uses toJSON to hide it since Object.defineProperty isn't guaranteed:

That toJSON hack was added specifically to support the now-defunct jquery-datalink plugin, and it turned out that wasn't enough to allow it to be Object.observe. I'd be okay with removing that in the 3.0 compat branch for symmetry. Our release notes can provide enough info to allow people to make their own fixes for these edge cases.

Another thought @gibson042, what if we use the same { toJSON: jQuery.noop } initial object for master? That gives us the same behavior across both.

That sounds good.

Is the toJSON/noop something I should do in this PR?

@jbedard oh that would be awesome. We're preoccupied with a release the last couple of days. You got the gist of it right? We want the behavior identical in the two branches. I think we're already set for a unit test, you just gotta put it back. 😈

I'll give it a shot, hopefully tonight.

But now that I look at how 1.x does it... In 1.x .data on an object actually puts the data on the object, so the data object can have the toJSON put on it. But in 2.x putting data on an object works the same as elements where only the uid is put onto the object. We could change 2.x to be more like 1.x but that makes this a bigger change, and starts to cross with #1428...

Hm. Looks like this was never landed: #1428

As soon as this lands, I will rebase the other.

If #1428 will land I think that is a better place to introduce the toJSON. Should this PR go back to defineProp for now though?

I guess so...sorry for the confusion @jbedard!

Updated to use defineProp again for non nodes.

The no-defineProp commit for reference: jbedard@94ddf72

It would be great if #1428 could land as well to remove defineProp. I think that will also make compat/master more aligned? Might even be able to port the entire 2.x Data to compat once it doesn't use any ES5..?

I rebased #1428 earlier today, so should be good to go

It's been a while. Is everyone ok if we land this?

Looks like @jbedard needs to re-sign the CLA with the email he used for the commit, but otherwise YES, let's get this puppy landed! 🐶

I've resigned with the email of the commit. Does @jquerybot need something to trigger it?

@jbedard Looks alright. Thanks! The bot doesn't have the ability to re-check PRs yet. (jquery/jquery-license#2)

As promised, data-as-expando has been rebased

Hm. Looks like I lost something there. Hold the line please.