Animate optimizations

[dcherman]

In ngRepeat, we can avoid an extra level of jqLite wrapping by passing the raw node. Internally, it looks like $animate already supports this as it re-wraps the node anyway.

We can also optimize the calls to determine whether or not an animation should be allowed on a given element or not by using the lower level .data() api rather than .fn.data(). When jQuery is loaded, using the latter represents a significant amount of overhead due to data attribute parsing.

A more extreme change that I want to propose is that we bypass any type of data API entirely and simply start using Symbol and sticking the metadata on the node itself. There is already precedent for that in the codebase (see ngRepeat where it marks nodes to be deleted like that). In browsers that don't support Symbols, a simple polyfill can be used to generate a key that will not collide.

[dcherman]

Another potential optimization for perf here is just to stop using jQuery or jqLite at all. No functionality from either library is used other than .data() and .parent(). The former is addressed by this PR and potentially by the original comment, and the latter is trivial since parentNode just works across all browsers.

Doing that along with this PR looks to get us close to a 75% speed improvement with no behavioral changes in this function.

[Narretz]

Thanks for he PR. Good stuff. What's your test setup for the performance tests?

[dcherman]

At the moment, it's nothing overly complicated. I have some use cases internally that we know to be slow during initial compilation, so I just run those use cases 5 times with the profiler running, see where our time is being spent, and optimize.

For reference:

Before:

After:

So in this specific function, the self time is almost entirely unchanged which isn't too surprising since no actual logic was changed, but time spent in child calls was nearly eliminated compared to what it was previously

Edit: The "after" contains changes not seen in this PR, but were proposed including the change to use expando properties on the element directly rather than $.data, and swapping out .parent() for .parentNode

[Narretz]

I've also made some tests and your version is approximately 7% faster in an ngRepeat with a 500 elements (that don't actually have animations). The actual improvement depends very much on the number of animatable elements, but even 7% is nothing to sneeze at.

[Narretz]

I just noticed that one commit addresses jquery specifically. I haven't tested that yet.

[dcherman]

@Narretz If you're game, I'm happy to look into adding the other changes that I described in my previous commit. The one I'm most curious about is whether or not you'd be open to using expando properties on elements rather than .data(). The parentNode optimization is relatively straightforward and well supported in all browsers.

The biggest concern that I have with using a Symbol or otherwise "private" property is codepaths like this which might miss those added properties.

[dcherman]

Btw, the jQuery path is where you're going to see huge performance gains. The reason it's so expensive is that part of the .data() contract is that it parses data- attributes when you ask for a value via that API. When you're iterating over N elements, each with their own set of attributes, that can add up to a lot of time.

[Narretz]

@dcherman What's the parentNode change about again? If it's straightforward, go ahead and add it (in another commit)
Using expando properties should go into a separate PR, because this here is already a good (and can be backported without trouble)

[lgalfaso]

Hi all,
I think there are multiple things being talked here.

• We know that creating jQuery wrappers is expensive, this is one of the reasons why using jqLite is faster
• When we can avoid using jQuery/jqLite and doing the operations on the nodes directly (and this does not affect the readability of the code), then we should go for it
• Adding properties to the DOM nodes have a performance hit on many browsers that promote DOM nodes to normal objects when a non-DOM property is set to them. Promoted nodes are much slower than normal nodes

The last point is the one that we should be more careful about when doing any change to data

Other than that, all these improvements are welcome!

[dcherman]

@lgalfaso Interesting, I didn't know about that de-opt when adding expando properties. Does the number of properties added matter, or even a single one? At least until a WeakMap is generally available, both jQuery and jqLite have to use expando properties for their respective data caches, so those nodes may have been promoted to normal objects already.

[lgalfaso]

A single non-DOM property is enough for the object to be promoted. Do not
know if using Symbol makes a difference, but these changes need to be
tested (if possible, with multiple browsers)

On Tuesday, February 9, 2016, dherman [email protected] wrote:

@lgalfaso https://github.com/lgalfaso Interesting, I didn't know about
that de-opt when adding expando properties. Does the number of properties
added matter, or even a single one? At least until a WeakMap is generally
available, both jQuery and jqLite have to use expando properties for their
respective data caches, so those nodes may have been promoted to normal
objects already.

—
Reply to this email directly or view it on GitHub
#13879 (comment).

[dcherman]

@lgalfaso Happy to do some tinkering. Do you have any resources about that perf hit? I'm not sure what the observable side effects would be; slower DOM methods, higher memory usage, etc. The only references to expando property problems that I've found so far are related to the old memory leaks in IE < 8, but that's generally no longer relevant.

FWIW, not only do jQuery/jqLite do this, but several places in ngAnimate use the HashMap implementation from Angular core that will result in expando properties being put on DOM nodes. Since many directives use $animate for their operations, that means that a lot of places that'd be considered hot would run into this (ex. ngRepeat).

Other than that, I pushed the change to use parentNode that I was describing earlier.

[jbedard]

I believe every time you add or remove a property to a DOM node the browser essentially creates a class (see: jquery/jquery#2479 (comment)).

[lgalfaso]

@dcherman #13879 (comment) is one reference to this. This affects all modern browsers as there are many optimizations possible when DOM nodes are not JS object

[gkalpak]

FWIW, calling jqLite's .data() as a setter does already add the ng339 property to the DOM node.

[dcherman]

Interesting. WeakMap has pretty good support actually (Chrome, FF, IE11+), but it looks like V8 may have issues related to garbage collection that would potentially make using it as a hot data store (jqLite.data) potentially a bad idea, so it's probably not worth the overhead of diverging codepaths right now.

@lgalfaso Let's not let this investigation hold this PR up since we still get a lot of win out of this. I'm crawling through the Blink code to try and determine if this situation really is special cased, or if it just follows the standard rules of hidden classes with V8 objects (probably not since the DOM isn't implemented in JS...yet). I'd be curious to know if there's a difference if we were to pre-assign something like HTMLElement.prototype.ng339 = "" (ng339 being the jqLite cache key). Would that cause all elements ever to follow this de-opt path? I don't know enough about the differences between representations of DOM and V8 objects to know if they follow the same rules. Maybe @jbedard has some insight to know if there's a difference there.

[jbedard]

I think the bigger issue blocking the use of WeakMap is that lookups are slow?

I'm not sure about modifying the prototype but if you find anything I'd be curious! Note that often the perf hit is with GC/memory usage - if you add/remove a property from a DOM node it seems to copy the node to a new object (which often consumes more memory then the original object). But maybe if the HTMLElement.prototype already had that property then this copying would go away? Or maybe it would cause every node to be copied on construction instead of only when needed?

[lgalfaso]

landed this PR

[lgalfaso]

I think we can still use this PR for other performance changes, but what is already here was ready to be landed

[dcherman]

Cool stuff. I'm not going to pretend to be a v8/blink/C++ expert (or even a novice for that matter), but it looks like the bridge from DOM -> v8 is contained in https://chromium.googlesource.com/chromium/blink/+/master/Source/bindings/core/v8/V8DOMConfiguration.cpp. I also have no idea how this is implemented in either Firefox or IE.

Unless I'm barking up the wrong tree which is entirely possible, the implementation on that file means that means that Blink will essentially generate V8 backed classes, so they should follow similar rules to plain old JS hidden classes after the initial construction. That means that setting a prototype property as in my previous example is entirely useless since prototypes and instances don't share the same hidden class.

@jbedard Nope, WeakMap lookups are actually plenty fast. Just ran a jsPerf on them across Chrome, FF, and IE11 and each of them has perfectly acceptable ops/sec (in the millions or tens of millions). The bigger issue is that per the issue I linked earlier, WeakMaps aren't weak enough, they're kinda strong in v8. Apparently you need two full-scale GCs in order for keys to be cleaned up properly; incremental ones don't work. That means that while memory won't technically leak, you may end up with more bloat than necessary before things get GCed, and that GC pass would take longer than it normally should since it's got more work to do.

[dcherman]

So knowing that the bridge to v8 uses the same internals as other objects, we can do some simple tests using some of the v8 natives

var a = document.createElement('div');
var b = a.cloneNode();

// True, indicating both A and B share the same hidden class
console.log(%HaveSameMap(a, b));

b.foo = "bar";

// False.  B has a different hidden class
console.log(%HaveSameMap(a, b));

a.foo = "bar";

// True.  A and B now share the same hidden class again
console.log(%HaveSameMap(a, b));

A lot of this exploration is just for fun; as of jQuery 3.0 where they started attaching data straight to the DOM as an expando rather than using defineProperty, there shouldn't be that significant of a perf difference from just using a Symbol/expando ourselves, so it's probably not worth the potential breakage.

Next up, would be interesting to know what the implications of changing the hidden class are memory-wise. That should be able to answer the question posed previously of whether or not all of the properties of the node are copied to whatever internal representation exists in V8, or if the existing structure is re-used.

@jbedard Since you were interested.

[dcherman]

Last update.

If you then inspect what exactly transitioning hidden classes entails, it looks like it's expected that as you transition from class A -> B, the in-memory layouts are compatible. If you transition from B -> A, that's a more generalized transition, the layouts should be compatible already.

https://github.com/v8/v8/blob/master/src/objects-inl.h#L5350-L5368

Again I really have to stress than I'm not even a C++ novice to take everything I've said with multiple grains of salt, but since the JSObjects share the same layout, I don't believe that they need to make copies of the object; they're visiting the same location in the heap. If you think about it, that makes a ton of sense anyway.

If doing a.foo = "bar" is enough to trigger a transition, can you imagine what would happen when you do Object.assign(a, { foo: 1, bar: 2 ... })? That seems like it'd be incredibly memory inefficient if they were to copy on every transition rather than sharing where possible.

Still nothing really actionable is going to come out of this, but it was definitely interesting to dive into the v8 internals a bit.

[jbedard]

Interesting stuff.

I haven't looked at the C++ code or anything, but I don't think the in-memory layouts will always be compatible. When transitioning from the normal classes to custom ones (because you add a property to a DOM) it often consumes significantly more memory, so it couldn't possible be using the same block of memory...?

[Narretz]

@dcherman Have you tested the differences without jquery, too? I've tried some tests today, and for some reason with the 3 changes, it's always a bit slower.

[dcherman]

@Narretz I'll do some testing without jQuery loaded a bit later today. I would be very surprised if this had any negative side effects on jqLite performance since I'm literally doing a subset of the work that was done before

[dcherman]

@Narretz So I've done some testing, but it's kind of inconclusive. I've ran tests where it's slower, and I've ran tests where it's faster, probably attributed to just normal variation.

The only thing I see in the code as a potential slow down is that we're changing the type of parentElement from a jqLite object to a raw DOM node if one was passed in. Maybe that messes with some v8 optimizations internally; not sure.

What do your tests look like? I'm just making a list of 1000 items, each of them calling attrs.$addClass('foo');

[Narretz]

@dcherman That's odd.

I've tested with the benchmarks in #13976
I thought maybe the test setup is causing these numbers, but it's not reassuring that you also have these inconclusive numbers.