Rework component update logic avoiding memory allocations

[mrxz]

Description:

The component update logic is very complicated due to the many cases it needs to take care of. There are multi-property, object-based single-property and single-property components. The schema for multi-property components may be dynamic. There are three methods used to update (or as a reaction to an update): updateProperties, resetProperty and handleMixinUpdate. The updateProperties accepts both CSS-style strings as objects, and optionally clobbers. Combined this means that there are 6 update interactions on 4 different component types, resulting in 24 different cases. And that's not even taking into account the case of updates happening before/after the entity has initialized.

The goal of this PR was two-fold: making the component update logic more readable and reducing memory allocations. As a nice side-effect the overall performance of setAttribute has improved a fair bit.

	set-attribute test	set-attribute test (w/ type: 'vec3')
Firefox 122 (before)	7.28 µs/op	25.91 µs/op
Firefox 122 (after)	4.94 µs/op	6.18 µs/op
Chrome 120 (before)	7.84 µs/op	29.72 µs/op
Chrome 120 (after)	6.20 µs/op	6.67 µs/op

The three methods mentioned above now all effectively follow the following pattern:

    // Perform changes and recompute (either specific properties or entire data)
    this.recomputeData();
    // Handle schema changes
    this.updateSchemaIfNeeded();
    // Call update in case a change is detected
    this.callUpdateHandler();

Memory changes

• Reduce the number of objects from the objectPool a component needs:
  • Remove the need for parsingAttrValue. This intermediate object held the (raw) values that would end up on attrValue. Instead the parsing is done onto attrValue (through a proxy object)
  • Remove the need for previousOldData. Instead oldData is passed and if-and-only-if a recursive update occurs, a new oldData object is retrieved from the objectPool and the old one recycled once done, ensuring it remains valid for the duration of the update call and supports arbitrary nesting (though recursive setAttribute isn't ideal)
• The inheritance chain (schema defaults, mixin values, attrValue) in buildData was handled through the use of copyData and extendProperties both of which clone objects by default, even if said values might not end up in the data. Instead, getComputedProperty handles the same inheritance chain, but always returns the reference, leaving it up to recomputeProperty to perform the copy/clone (if needed)
• Alter the semantics of parse(Property) to allow passing in an (optional) target value that can be re-used. This allows the parse method to handle parsing, cloning and copying in one.
  • ⚠️ This is a change in behaviour, of the built-in parsers only the coordinates didn't behave as-such. Users might however have parse functions that do not behave as such. If they only support parsing (string -> output) and cloning (output -> clone) it would still work. If instead of cloning it becomes the identity (output -> output), this only works if re-using by reference isn't in issue for said type. Otherwise it'll be a breaking change
  • This also allows efficient copying from data into oldData and from the computed value into data

Note: Effectively the data and oldData will hold and own object based properties. The parse method can both allocate a new one (initially) or re-use the one already present. This is versatile approach as the exact behaviour is up to the propertyType. This also (partially) addresses #5181 as coordinates can interpret Vectors just fine and will copy from them, instead of using references. I believe this was also the original behaviour but broke when Three.js switched to classes.

Performance changes

• Changes to data are no longer performed through deep-equal, instead the schema is used to pick the corresponding equals function per property.
  • ⚠️ This is a change in behaviour and won't pick up on changes made directly to data outside of it's known properties, e.g. adding a this.el.components.dummy.data.someVector.myField = true won't be picked up as a change (nor appear in oldData)
• Avoid calling updateSchema unless needed. Since the changes are detected on-write, we know when a schemaChange property has changed

Bug fixes

• Both resetProperty and handleMixinUpdate did not take schema changes into account, now they do
• resetProperty would reset the property to its schema default, ignoring any mixed in values (inconsistent inheritance)
• Due to the re-use of parsingAttrValue, using setAttribute with a CSS-style string could often lead to updateSchema being called even when no schemaChange properties were included or changed
• There are various workarounds to avoid double parsing, but these were inconsistent between single-property and multi-property components.
• Fixed inconsistent logging of "unknown property" warnings when schema changed (double logging, false positives).
• Fixes Default value returns NaN for numeric, single-property component schemas in 0.7.* and 0.8.0 #3442
• Fixes Updating a line component using a cached THREE.Vector3 fails #3681
• Fixes FlushToDom causing the environment to crash in Aframe 0.9.0 #4014
• Fixes Single-prop value not parsed when using utils.entity.setComponentProperty #4018
• Fixes Single property schema where the property is an array does not work (broken since 1.0.0) #5242

Next steps

This PR ended up being quite large and I can imagine hard to review. In the coming days I'll try to create a separate PR that introduces a bunch of tests (including ones failing on master) to document both the fixes and changes in behaviour.

There are bound to be more behavioural changes when tested in the wild. Especially for users that directly mutate .data or even use internal methods or fields.

Following this PR, there are a couple of other changes that could follow:

• Splitting the implementations for single- and multi-property components (no runtime overhead and easier to read).
• Unify registerSystem and registerComponent, effectively turning systems into components.

[mrxz]

This test was specifically introduced to cover a regression, however I now believe the covered behaviour is actually a bug. The partial combining of object-based single-property data only occurs when applied before the entity is initialized, not after. This inconsistent behaviour isn't ideal, so either it should always combine or never.

If backwards compatibility to this extend is needed, I can introduce a quirk that explicitly causes the old behaviour when updating a object-based single property before initialization.

[mrxz]

This test has been re-arranged since the captured values are by reference. Properties on oldData are only valid for the duration of update. Now the relevant assertions are taken during the update.

On master this tests 'works' because the referenced object hasn't been changed yet. Making the chain longer (say three updates), it also shows the same problem. Now this is a small behavioural change, but users shouldn't be keeping references to either data or oldData even before this change.

[mrxz]

The value of attrValue is only set once a value is set. Without setting any properties, attrValue isn't allocated (saving memory). This test inspects the attrValue which is internal and thus this change

Furthermore the test only cares about whether or not the selector property el is being cloned or not. So setting a different property to ensure attrValue is allocated has no bearing on the tested behaviour.

[mrxz]

The outcome of recomputeData is roughly the same as buildData. One difference being that recomputeData doesn't return the updated data value, requiring these test cases to be changed accordingly.

[mrxz]

Moved and adapted from component.test.js where it tested the parseAttrValue method. Since this PR causes it to only be used by a-mixin, the tests were moved as well.

[dmarcos]

needs rebase

[dmarcos]

needs rebase

[dmarcos]

Is still the plan to break this PR on smaller ones or should I take it as is?

[dmarcos]

This is done?

[mrxz]

This is done?

The PR is in a complete state, so you could start reviewing it. I know I said I would open a PR with additional tests, but sadly haven't gotten around to it. That shouldn't change the implementation, though, mostly cover the bugfixes listed above.

[dmarcos]

Thanks! Great work!

[diarmidmackenzie]

Haven't had a chance to look in detail, but this looks like a substantial improvement to an area of A-Frame that was rather patchy and inconsistent (and hence frequently hard to work with)

Thank you @mrxz. Awesome vision and execution!

[mrxz]

Thanks, I'm quite pleased with how it turned out. It's still a complex part of A-Frame, simply by the fact that there a so many possible code paths when updating components, but the code should be easier to follow and behave more consistent. Some inconsistency (e.g. between single- and multi-prop) have explicitly been added for backwards compatibility and now "stick out" instead of being unintended side-effects of the different implementations.

In any case, if you do happen to look at it in more detail or notice any changes or regressions after updating, let me know.