reduce memory allocation in component update core codepath #3772

ngokevin · 2018-09-25T13:46:58Z

Description:

Rather than create a new object each time to create a new oldData, just have two objects representing oldData and data. Also when updating, rather than create a new data object, have a newData object that is used for holding the new data which then gets copied into data. So in total, only use three objects: oldData, data, newData.

Changes proposed:

Adapt style-attr implementation to be able to pass in an object for reuse instead of creating a new one each time. Also remove lots of callbacks, Object.keys() array creations from their code. Also tweaked to do prettier stringifies. We manage this forked dependency now.
Use object pooling in core component code to reduce object allocations. Recycle them when component is removed.
- 1. Remove object allocations on each update for deep cloning data to oldData.
- 1. Remove object allocation on initialization for initial oldData.
- 1. Remove object allocation on each update during buildData for default value copying.
- 1. Remove object alloaction during buildData, was using Object.keys.
Add some helper properties to component (isSingleProp, isSinglePropObject, isObjectBased)
Update our styleParser utils a bit to not allocate object + array + use callbacks to convert keys to camelCase, and to cache regexes.

Updated to not delete the keys when recycling an object, but just set every key to undefined. This means object pooling best used when objects share a common schema, in case we iterate over keys. So I create different object pools for each type of component, since their object keys will be the same. I also update schema parser to ignore undefined keys.

Before

After

Improvements: See the updateProperties and updateCachedAttrValue no longer on the list of frequent memory allocators.

Still need to polish/comment a bit more (and try to remove the clones for default values). But up and working again.
Tests

ngokevin · 2018-09-26T11:16:47Z

Ready to go. Did another test with the /performance/animation-set-attribute stress test to measure memory. These recordings were taken over 4 seconds. We save the following allocation calls:

buildData (component) (~400KB)
getKeyValueChunks (style-parser) (~160KB)
cloneData (~80KB)
updateProperties (~28KB)

With the particular stress test, it does lose out on ~5FPS (20FPS vs 15FPS), but it's also an unrealistic example since we wouldn't use setAttributes that hard and often for animations. Dodging GC is more important because GC will lose you entire frames.

We could perhaps have an option for components to not do any diffing, cloning, comparing, and just always run the update method no matter what, and let the component sort out data changes. We are probably losing a lot of time making sure redundant calls don't get processed.

Before

After

arpu · 2018-09-30T11:08:21Z

@ngokevin i would like test this change, how do you generate the memory statistics?

dmarcos · 2018-09-30T16:17:06Z

src/components/camera.js

@@ -79,6 +79,11 @@ module.exports.Component = registerComponent('camera', {
      // Camera disabled. Set camera to another camera.
      system.disableSpectatorCamera();
    }
+
+    this.savedPose = {
+      position: el.getAttribute('position'),


I think we have to allocate new objects here. You will be holding a reference to the entity shared objects that will change.

I just removed it, it wasn't used.

ngokevin · 2018-09-30T17:55:50Z

browser memory tools

… component

dmarcos · 2018-10-03T14:43:32Z

It would be nicer if this PR could
be broken in smaller PRs. It touches a lot
of files and it’s hard to follow and see implications

dmarcos · 2018-10-03T14:45:29Z

src/components/obj-model.js

@@ -21,12 +21,16 @@ module.exports.Component = registerComponent('obj-model', {
  update: function () {
    var data = this.data;
    if (!data.obj) { return; }
-    this.remove();
+    this.resetMesh();


Unrelated to the purpose of the PR?

fixes a test

dmarcos · 2018-10-03T14:46:24Z

src/components/material.js

-    var shader = newShader || currentShader;
-    var schema = shaders[shader] && shaders[shader].schema;
+    var currentShader;
+    var newShader;


unrelated to PR?

fixes test, checks for data / oldData existence

dmarcos · 2018-10-03T14:46:51Z

src/components/geometry.js

   */
  updateSchema: function (data) {
+    var currentGeometryType = this.oldData && this.oldData.primitive;


unrelated to PR?

yes, it checks for oldData rather than data

dmarcos · 2018-10-03T14:52:42Z

styleParser.js and component.js changes could be separate PRs

ngokevin · 2018-10-03T20:00:24Z

It's hard and effort to break up. They both relate to the object pooling and both tie together. There's only two major changes:

remove allocations from style parser by passing an object in (managed with object pool)
remove allocations from component.js by removing the cloning (managed with object pool)

I detail listed all the changes in the original description. It touches lots of files because it's a change deep in the core of A-Frame.

arpu · 2018-10-06T10:06:26Z

this breaks my little app with

body.js:122 Uncaught TypeError: Cannot read property 'toUpperCase' of undefined at initBody (body.js:122) at HTMLElement. (a-node.js:300) at a-node.js:131 initBody @ body.js:122 emit @ a-node.js:300 (anonymous) @ a-node.js:131 Promise.then (async) load @ a-node.js:128 load @ a-entity.js:255 (anonymous) @ a-scene.js:591 setTimeout (async) play @ a-scene.js:590 attachedCallbackPostCamera @ a-scene.js:128 (anonymous) @ a-scene.js:102 emit @ a-node.js:300 checkUserCamera @ camera.js:98 (anonymous) @ camera.js:58 emit @ a-node.js:300 setObject3D @ a-entity.js:166 init @ camera.js:28 updateProperties @ component.js:315 updateComponent @ a-entity.js:489 updateComponents @ a-entity.js:463 (anonymous) @ a-entity.js:259 (anonymous) @ a-node.js:130 Promise.then (async) load @ a-node.js:128 load @ a-entity.js:255 (anonymous) @ a-entity.js:84 emit @ a-node.js:300 (anonymous) @ a-node.js:131 Promise.then (async) load @ a-node.js:128 load @ a-assets.js:83 (anonymous) @ a-assets.js:70 setTimeout (async) value @ a-assets.js:66

https://github.com/donmccurdy/aframe-physics-system/blob/master/src/components/body/body.js#L64

ngokevin · 2018-10-06T11:07:14Z

can you log this.data, and more information if you're updating it, initializing it...a live example would be great

arpu · 2018-10-06T11:42:26Z

here is a live example https://codesandbox.io/s/5k1wxylxkx

ngokevin · 2018-10-07T02:59:02Z

Thanks, possible to have one with the physics as an unminified project file so I can poke around in it?

arpu · 2018-10-08T13:44:34Z

https://codesandbox.io/s/vv6jm1l06l

with unminified physics

arpu · 2018-10-12T09:27:47Z

@ngokevin should i open a new bugreport?

ngokevin · 2018-10-12T09:28:31Z

thanks sure

ngokevin added the work in progress label Sep 25, 2018

ngokevin mentioned this pull request Sep 25, 2018

object pooling in component core, optimize style parser to reuse object #2951

Closed

ngokevin added this to the 0.9.0 milestone Sep 26, 2018

ngokevin force-pushed the objectpool2 branch 2 times, most recently from 9ae6222 to 8af4a5b Compare September 26, 2018 10:30

ngokevin added needs review and removed work in progress labels Sep 26, 2018

ngokevin changed the title ~~[WIP] object pooling in component core, optimize style parser to reuse object~~ reduce memory allocation in component update codepath Sep 26, 2018

ngokevin changed the title ~~reduce memory allocation in component update codepath~~ reduce memory allocation in component update core codepath Sep 26, 2018

ngokevin force-pushed the objectpool2 branch from 8af4a5b to 5052818 Compare September 26, 2018 10:47

dmarcos reviewed Sep 30, 2018

View reviewed changes

ngokevin added 6 commits September 30, 2018 10:56

object pooling in component core, optimize style parser to reuse object

8ce5d96

improve pooling by not using key delete, make a pool for each type of…

f262c63

… component

fixes to component optimizations

6a9ee26

continue rebase, fix tests

ece2fe8

fix obj-model calling .remove from another handler to fix tests

f87c296

remove unused savedPose from camera.js

784aafb

ngokevin force-pushed the objectpool2 branch from 1e9660e to 784aafb Compare September 30, 2018 17:57

dmarcos reviewed Oct 3, 2018

View reviewed changes

dmarcos merged commit 6deaea0 into aframevr:master Oct 4, 2018

arpu mentioned this pull request Oct 15, 2018

commit 6deaea0 breaks aframe-physics-system.js with Cannot read property 'toUpperCase' of undefined #3808

Closed

diarmidmackenzie mentioned this pull request Feb 14, 2023

Single property schema where the property is an array does not work (broken since 1.0.0) #5242

Closed

This was referenced Sep 8, 2024

[Snyk] Upgrade: aframe, nuxt, vuetify peterclemenko/vuetify-aframe-issue#43

Open

[Snyk] Upgrade: aframe, nuxt, vuetify peterclemenko/vuetify-aframe-issue#44

Open

[Snyk] Upgrade: aframe, nuxt, vuetify peterclemenko/vuetify-aframe-issue#45

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

reduce memory allocation in component update core codepath #3772

reduce memory allocation in component update core codepath #3772

ngokevin commented Sep 25, 2018 •

edited

Loading

ngokevin commented Sep 26, 2018 •

edited

Loading

arpu commented Sep 30, 2018

dmarcos Sep 30, 2018

ngokevin Sep 30, 2018

ngokevin commented Sep 30, 2018

dmarcos commented Oct 3, 2018

dmarcos Oct 3, 2018

ngokevin Oct 3, 2018

dmarcos Oct 3, 2018

ngokevin Oct 3, 2018

dmarcos Oct 3, 2018

ngokevin Oct 3, 2018

dmarcos commented Oct 3, 2018

ngokevin commented Oct 3, 2018 •

edited

Loading

arpu commented Oct 6, 2018

ngokevin commented Oct 6, 2018

arpu commented Oct 6, 2018

ngokevin commented Oct 7, 2018

arpu commented Oct 8, 2018

arpu commented Oct 12, 2018

ngokevin commented Oct 12, 2018

reduce memory allocation in component update core codepath #3772

reduce memory allocation in component update core codepath #3772

Conversation

ngokevin commented Sep 25, 2018 • edited Loading

Before

After

ngokevin commented Sep 26, 2018 • edited Loading

Before

After

arpu commented Sep 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngokevin commented Sep 30, 2018

dmarcos commented Oct 3, 2018

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

dmarcos commented Oct 3, 2018

ngokevin commented Oct 3, 2018 • edited Loading

arpu commented Oct 6, 2018

ngokevin commented Oct 6, 2018

arpu commented Oct 6, 2018

ngokevin commented Oct 7, 2018

arpu commented Oct 8, 2018

arpu commented Oct 12, 2018

ngokevin commented Oct 12, 2018

ngokevin commented Sep 25, 2018 •

edited

Loading

ngokevin commented Sep 26, 2018 •

edited

Loading

ngokevin commented Oct 3, 2018 •

edited

Loading