Data: Feature detect and use `Symbol` if present

[dcherman]

JSPerf: http://jsperf.com/defineproperty-vs-symbol-for-privacy

I've noticed that in much of my code, Data.key is often near the top of CPU time for initialization when hundreds or thousands of elements have their data initialized (as is sometimes the case using ngAnimate in Angular).

By feature detecting the presence of Symbol and using it rather than a non-enumerable property, this function went from like 70-100ms CPU time to 0.3 since not only does it not have the overhead of define property, but we can avoid the try/catch in that codepath as well.

[dmethvin]

That particular jsperf looks bogus, most likely the Symbol case is being completely elided? It wouldn't surprise me if it was a better/faster solution though, we know defineProperty is a dog. Is that the path you're taking now though? Sounds like animations should be on DOM elements.

So would the idea be to replace the plain assignment for DOM elements as well as the defineProperty we use for everything else whenever Symbol() is available? Just wondering if we can get rid of some cases based on the supported browsers, or whether we add another one.

[mgol]

Current Data code is very different than what's in jQuery 2.1.4; in particular, there is no try-catch around defineProperties; you'd need to check that out. Also, as @dmethvin pointed out, this test case is bogus as the Symbol assignment gets jitted away in modern browsers, you'd need to create a patched jQuery version & compare method invocations to see the real difference.

That said, this seems more of a use case for WeakMap than Symbol. @rwaldron wrote this whole code having WeakMaps in mind after all. :) @jdalton's experiments in lodash might suggest this could make the Data module faster but we'd need to check.

[dcherman]

Sweet, I should have checked out master before filing this; I didn't realize that the define property path had been eliminated with DOM Nodes along with the try/catch. That should essentially be equivalent to the Symbol path then since at that point, it's just a property access.

Have WeakMaps been optimized to the point that they're usable yet? I recall someone doing perf tests a while back and they were pretty slow; hopefully that's been fixed! Maybe @jdalton knows?

[jdalton]

Weak maps are a bit touchy in V8 as repeated use (like that in a micro-benchmark) can still cause prolonged GC pauses. I have a heuristic in place to short circuit my use if they get hot (my use isn't for critical machinery just as a performance optimization to store metadata about function wrappers to avoid repeated wrappings).

Anything you do, symbols or weak maps, will likely be faster than defineProperty use. It's the worst. I switched to weak maps because they were less expensive than defineProperty.

[dmethvin]

Seems like we don't really need WeakMap if we're okay with having the data garbage-collected once the element goes out of scope, when it's attached via either a plain property or a Symbol. What would be the advantage?

[mgol]

@dmethvin We can't use regular assignment for non-nodes as this would be observable too much so WeakMaps fit there... but that's not the main use case of jQuery data module and on nodes we use regular assignment, right. So maybe we don't need to do anything here currently; we need a non-WeakMap path for objects anyway and since they're not the primary use case, that might be too much.

[jdalton]

@dmethvin Yah, I was speaking to weak maps as they were mentioned in the thread. I wasn't giving an endorsement or push for them to be used in jQuery. The advantage of weak maps is there's no side effects to the element (no bolted on property).

[dmethvin]

Okay, just wondering if I had missed something. If we could go completely one particular way it would improve the consistency, but it seems like it will be a while before we could do that with any of the possible solutions. From a performance standpoint, getting rid of defineProperty for the really common element case seems like the biggest win, and we've got that now.

[dcherman]

Yep, everything looks sweet; I should've checked master before filing this. My bad.

If you want, we can still use a Symbol since it'd get closer to truly hiding properties on Nodes and would avoid the defineProperty path on normal objects, but there would be pretty much no perf impact one way or the other for the most common case (Data on Elements).

[mgol]

We've decided to not use Symbols. It would have to be an additional code path and it wouldn't save performance on nodes where we now use a regular property. It might only save perf on objects which is not a dominant use case here.