Refactors component updates to minimize the number of string parsings…

[dmarcos]

Attributes updates is the most common operation in a-frame scenes. I've been looking for performance bottlenecks on setAttribute/getAttribute calls. Using the DOM to store the current state of the component incurs in a lot of component data stringifications (to feed the DOM attribute via HTMLElement.prototype.setAttribute) and parse calls to recalculate the component data from the attribute string or when getAttribute is called by the application logic. By caching the parsed attribute corresponding to the component we can save a lot of stringify/parse calls. The overhead is specially noticeable on scenes with a lot of dynamic behavior. I've added a stress test to illustrate the problem and show the performance gains. In the stress-animation demo I create a lot of elements with animations that will call setAttribute continuously. On my machine and without the patch the example runs in the lows-mids 20s FPS. With this patch applied it goes up to the lows-mids 30s FPS. That's approximately a 50% perf gain.

[cvan]

can you add the issue number (#1311) to the commit message?

[dmarcos]

This is the final design this patch has converged to:

1. We don't update the DOM anymore to reflect the state of a component.
2. The attribute value is cached in a parsed form within each component. This both saves stringification calls whan calling getAttribute and parse calls when calling setAttribute('component', 'property',value)
3. There's an updateDOM method in the entity and component to write back the state to the DOM with the cached value. This can be used by applications that rely on the DOM like our sceneVR or our future editor.
4. When a component is initialized we write the attribute to the DOM without the property values. You will see something like this: <a-entity position rotation scale camera wasd-controls look-controls></a-entity>. One can still query for an entity with a specific component sceneEl.querySelector('[camera]') and standard methods el.hasAttribute and el.attributes remain functional.

[dmarcos]

The final gains for this patch on the included stress-animation demo are pretty significant. The example has 369 animations affecting 369 different entities. That translates to at least 369 setAttribute calls per frame. On my machine and without the patch the demo runs at low 20s FPS. With this patch applied it hits high 30s FPS. That's almost twice as fast.

[dmarcos]

Things to look for when reviewing this patch. I like the perf benefits but I'm worried about the form. The complexity of the code to handle all the different cases is phenomenal. It's brittle and very difficult to understand. I cannot keep the complete flow in my head. This patch has been an effort of massaging the code until it covered all the edge cases. This work would not have been possible without the unit tests we have. There should be a simpler more concise way to express the same logic. I will keep thinking about ways to simplify. Bringing more eyes to this patch will help too.

[ngokevin]

• Examples work
• react boilerplate works

[ngokevin]

Can remove.

[ngokevin]

We call it cachedValue in one place and attrValue in another. Should we call it cachedAttrValue everywhere?

[dmarcos]

I made a big clean up.

[ngokevin]

space after radius radius: 1, extra space before material

[ngokevin]

r+wc

[fernandojsg]

Just checked with the editor and after some small changes everything works fine! I tried to inject debug=true statically and dynamically and both seems to works as expected, so from my side is looking good.

[dmarcos]

All right. I'm going to merge this monster patch. Demos seem to work. I feel better about code quality and it's also much faster :)

[ngokevin]

Nice work 🎆

There're some unaddressed nits, but I can help with those if you want.

[dmarcos]

I changed stuff after the last review. Everything should be addressed.

[ngokevin]

GitHub shows some unaddressed diff lines, but not a big deal.