Skip type checking if an object is passed through setAttribute twice #2679

dmarcos · 2017-05-19T01:23:53Z

I'm trying to find ways to optimize setAttribute / getAttribute calls specially when they're called once per frame like for example in a component's tick method.

My first attempt is to disable type checking when setAttribute is called twice with the same object. Types are checked the first time and then the object is considered trusted. This would allow to construct your tick method in the following way:

  tick: (function () {
      var newRotation = {};
      return function () {
      var el = this.el;
      var currentRotation = el.getAttribute('rotation');
      newRotation .x = currentRotation.x + 0.1;
      newRotation.y = currentRotation.y + 0.1;
      newRotation.z = currentRotation.z + 0.1;
      el.setAttribute('rotation', newRotation );
    }
  })()

This aligns with the good practice of avoiding object allocations inside your render loop. This optimization yields a 30%-40% percent FPS improvement with respect to master when rotating 2500 cubes using a tick like the above one.

ngokevin · 2017-05-19T01:25:26Z

Doesn't the closure pattern cause objects to get shared across different instances of the same type of component? We've ran into issues with that before. Sometimes I wish for some of the advantages of closure-based component definitions.

ngokevin · 2017-05-19T01:34:36Z

src/core/component.js

 * @return {object} The component data
 */
-function buildData (el, name, attrName, schema, elData, silent) {
+function buildData (el, name, attrName, schema, elData, oldElData, silent, skipTypeChecking) {


We should just plop this function back into the component prototype as a method now, too many arguments.

dmarcos · 2017-05-19T01:36:47Z

You should not use the closure pattern to store state of the component. In this case is just an auxiliary variable to make a temporary computation so it's fine.

fernandojsg · 2017-05-19T07:58:45Z

30-40% improvements here in the 5k cubes scene

ngokevin · 2017-05-19T23:00:07Z

examples/performance/cubes/setup.js

@@ -0,0 +1,46 @@
+var URL_PARAMS = getUrlParams();


Might be nice to encapsulate this in a main component <a-scene main>

ngokevin · 2017-05-19T23:00:50Z

src/core/a-entity.js

   */
  updateComponent: {
-    value: function (attr, attrValue) {
+    value: function (attr, attrValue, clobber) {


I'd doc-string this to note .setAttribute passes this in

ngokevin · 2017-05-19T23:02:09Z

src/core/component.js

    var el = this.el;
    var isSinglePropSchema = isSingleProp(this.schema);
    var oldData = extendProperties({}, this.data, isSinglePropSchema);
+    var skipTypeChecking = typeof this.previousAttrValue === 'object' && attrValue === this.previousAttrValue;


I'd comment the conditions of type checking (If the passed data is an object and the object has not changed)

ngokevin · 2017-05-19T23:02:54Z

src/core/component.js

    var isSinglePropSchema = isSingleProp(this.schema);
    var oldData = extendProperties({}, this.data, isSinglePropSchema);
+    var skipTypeChecking = typeof this.previousAttrValue === 'object' && attrValue === this.previousAttrValue;
+    var currentAttrValue = !clobber && this.attrValue;


What's !clobber && this.attrValue mean? Can you add a comment

ngokevin · 2017-05-19T23:03:30Z

src/core/component.js

    var oldData = extendProperties({}, this.data, isSinglePropSchema);
+    var skipTypeChecking = typeof this.previousAttrValue === 'object' && attrValue === this.previousAttrValue;
+    var currentAttrValue = !clobber && this.attrValue;
+    this.previousAttrValue = attrValue;


We can define this in the constructor as well this.previousAttrValue = undefined

ngokevin · 2017-05-19T23:03:59Z

src/core/component.js

+    this.previousAttrValue = attrValue;

-    if (attrValue !== undefined) { this.updateCachedAttrValue(attrValue); }
+    if (!el.hasLoaded) {


Can you comment this clause?

ngokevin · 2017-05-19T23:05:30Z

src/core/component.js

+      this.updateSchema(this.buildData(attrValue, currentAttrValue, true));
    }
-    this.data = buildData(el, this.name, this.attrName, this.schema, this.attrValue);
+    this.data = this.buildData(attrValue, currentAttrValue, false, skipTypeChecking);


We could have buildData's third argument be an options object so we can be explicit vs passing in bools

buildData(attrValue, currentAttrValue, {skipTypeChecking: true, silent: false})

it's maybe premature if we only have two flags? It's also not a pattern we have in other parts of the code

Not concerned about the number of flags, but being explicit, because calling a function build(value, oldValue, true, false), hard to know what true/false does in that order.

But maybe not so we don't create another object.

buildData is not part of the public API and only used in two places in the whole code base so the chances of misuse are small. And also what you said. It's called only in two places but invoked on every component update so better to avoid new object instantations.

ngokevin · 2017-05-19T23:05:56Z

src/core/component.js

-    this.data = buildData(el, this.name, this.attrName, this.schema, this.attrValue);
+    this.data = this.buildData(attrValue, currentAttrValue, false, skipTypeChecking);
+
+    if (attrValue !== undefined) { this.updateCachedAttrValue(attrValue); }


Curious when would it be undefined?

when the component is defined via mixins for example. there's not an attrValue on the entity

ngokevin · 2017-05-19T23:06:33Z

src/core/component.js

+   * @param {boolean} skipTypeChecking - Skip type checking and cohercion.
+   * @return {object} The component data
+   */
+  buildData: function (elData, oldElData, silent, skipTypeChecking) {


nit: half the code calls it elData and half calls it attrValue, should we normalize?

ngokevin · 2017-05-19T23:07:47Z

src/core/a-entity.js

        el.updateComponentProperty(componentName, propName, propertyValue);
      }

-      /**


The logic here and below are handled by which pieces now?

There was a lot of redundancy in that logic. The flag clobber basically replaces all of it

dmarcos added the needs answers / feedback label May 19, 2017

ngokevin reviewed May 19, 2017

View reviewed changes

dmarcos force-pushed the cachObjectSetAttribute branch 3 times, most recently from c9c8cf3 to c0f190e Compare May 19, 2017 21:41

dmarcos added needs review and removed needs answers / feedback labels May 19, 2017

dmarcos changed the title ~~Skip type checking if an object is passed through setAttribute twice [WIP]~~ May 19, 2017

ngokevin reviewed May 19, 2017

View reviewed changes

dmarcos force-pushed the cachObjectSetAttribute branch 3 times, most recently from a8f06b3 to 25ca595 Compare May 21, 2017 21:13

Skip type checking if an object is passed through setAttribute twice

1a45e04

dmarcos force-pushed the cachObjectSetAttribute branch from 25ca595 to 1a45e04 Compare May 21, 2017 21:18

ngokevin merged commit 9eacbb6 into aframevr:master May 23, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Skip type checking if an object is passed through setAttribute twice #2679

Skip type checking if an object is passed through setAttribute twice #2679

Uh oh!

dmarcos commented May 19, 2017

ngokevin commented May 19, 2017 •

edited

Loading

ngokevin May 19, 2017 •

edited

Loading

dmarcos commented May 19, 2017

fernandojsg commented May 19, 2017

ngokevin May 19, 2017

ngokevin May 19, 2017

ngokevin May 19, 2017

ngokevin May 19, 2017

ngokevin May 19, 2017

ngokevin May 19, 2017

ngokevin May 19, 2017

dmarcos May 19, 2017

ngokevin May 19, 2017

dmarcos May 19, 2017

ngokevin May 19, 2017

dmarcos May 19, 2017

ngokevin May 19, 2017

ngokevin May 19, 2017

dmarcos May 19, 2017

Labels

3 participants

Uh oh!

Skip type checking if an object is passed through setAttribute twice #2679

Skip type checking if an object is passed through setAttribute twice #2679

Uh oh!

Conversation

dmarcos commented May 19, 2017

ngokevin commented May 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ngokevin May 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

dmarcos commented May 19, 2017

fernandojsg commented May 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Labels

3 participants

ngokevin commented May 19, 2017 •

edited

Loading

ngokevin May 19, 2017 •

edited

Loading