Data: Avoid ES5 defineProperty for speed

[jbedard]

As discussed in http://bugs.jquery.com/ticket/14839, removing the use of definePropert(y|ies) brings the jQuery2 data initialization up to speed with jQuery1 for DOM nodes. For non nodes this also switches from defineProperties to defineProperty which is a bit faster, saves an object and saves some bytes.

@gibson042 this has the changed you suggested in https://github.com/jbedard/jquery/commit/d44885b3603dd41ebc47ac889bc55a9a0da0fd4f

[rwaldron]

Thanks for contributing! There is an existing patch that will change how data is created and managed https://github.com/jquery/jquery/pull/1428

[jbedard]

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?

[rwaldron]

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

[markelog]

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

[jbedard]

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.

[rwaldron]

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.

[jbedard]

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 :(

[rwaldron]

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 :\

[dmethvin]

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

[jbedard]

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

[rwaldron]

@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)

[jbedard]

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

[rwaldron]

@dmethvin I think this is reasonable to land.

[dmethvin]

Awesome, thanks for the work on this everyone!

[jdalton]

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.

[rwaldron]

@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

[jdalton]

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.

[rwaldron]

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.

[bterlson]

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

[rwaldron]

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

[bterlson]

@rwaldron Yep, worth discussing.

[rwaldron]

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

[anba]

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.

[bterlson]

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

[jdalton]

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

[dmethvin]

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?

[dmethvin]

DAMMIT GITHUB

[dcherman]

If jQuery is following semver now, then the plain object support couldn't be removed until 4.0 which is a while out at this point.

Worth adding a //TODO: 4.0 note to remind you to remove the defineProperty path at that point? That said, WeakMap may even be widely available at that point which you'd probably want to evaluate anyway.

[jdalton]

If jQuery is following semver now,

From my understanding at the moment jQuery does not explicitly follow semver (in 1.x & 2.x land) the bump to 3.0 is starting point of official semver support and since 3.0 is a major bump breaking changes can be introduced.

That said, WeakMap may even be widely available at that point which you'd probably want to evaluate anyway.

WeakMaps are available in all modern browsers but are kind of painful at the moment. For example V8 has GC and performance issues associated with them. In my case I use them to store supplementary non-critical metadata but have to short circuit use when in a tight loop.

[timmywil]

@dcherman Why wouldn't removal in 3.0 satisfy semver? It is a major version bump.

[dcherman]

@timmywil @jdalton Unless I'm misunderstanding/misreading something, plain objects are being deprecated in 3.0, so they couldn't actually be removed until 4.0.

[jdalton]

shrug deprecators gonna deprecate.

[rwaldron]

WeakMaps are available in all modern browsers but are kind of painful at the moment. For example V8 has GC and performance issues associated with them

Confirmed.

The expando can (and should) go away in the future, but I stand by my comment here: https://github.com/jquery/jquery/pull/1668#issuecomment-59540184

[dmethvin]

I had proposed that we deprecate plain object functionality in 3.0, and didn't have a time frame for removal. However, being that we are going semver it would seem to be 4.0. In the meantime during 3.0 we wouldn't need to support additional plain-object scenarios or fix newly reported plain-object bugs.

I don't know how common the use of .data() is with plain objects.We could remove the exception here and have the expando property sitting out where it could be seen. That's a behavior change but then again it is 3.0. Thoughts? :white_check_mark: ? :x: ?

[rwaldron]

@dmethvin if we just pop the expando onto the side of the object and assume it's a node, we can defend that via the deprecation. I'm in.

[dmethvin]

Alrighty then. @jbedard do you want to update this PR to skip the check for it being a node?

[jbedard]

Sure, I can do that in a few hours hopefully.

[jbedard]

Updated. Note the changed tests though...

[markelog]

I had proposed that we deprecate plain object functionality in 3.0

actually, that was me, i always didn't get a credit for these stuff! :-)

[dmethvin]

Yep, I agree it was your suggestion @markelog! I should have had you open the ticket. :)

[gibson042]

A proper deprecation shouldn't introduce breaking changes to existing behavior. I think we ought to restore the tests and use Object.defineProperty on plain objects.

[mgol]

A proper deprecation shouldn't introduce breaking changes to existing behavior.

@dmethvin proposed that we don't deprecate but obsolete/remove it from 3.0.0 officially.

[dmethvin]

If this change prevented attaching data to plain objects I'd have more reservations, but all we're doing here is exposing the expando. Someone using a plain object would not be affected unless they did something like serialized the object to JSON or enumerated with for/in. I don't think this is a common usage, but if it is the time to introduce a breaking change would be in a major version bump like now.

[dmethvin]

For reference, this fixes gh-1728.

[gibson042]

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: https://github.com/jquery/jquery/blob/a4e31a8e2c214e4a021856b44a23368d103c169d/src/data.js#L97-L99

[dmethvin]

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.

[dmethvin]

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

[gibson042]

That sounds good.

[jbedard]

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

[dmethvin]

@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. :smiling_imp: