Fix memory leak from WebGLState light/shadow data from texture generation by jsantell · Pull Request #227 · google/model-viewer

jsantell · 2018-11-19T19:57:25Z

With the background-image demo often open on my machine, after long periods of time, my machine would hang, and had a hunch it had to do with the demo's infinite loading of new textures. In terms of severity, this would only occur when constantly changing textures (not a realistic scenario IMO) over a long period of time as small amounts of resources were being generated and not collected.

The leak wasn't terribly severe tracked leak down to cubemap generator, figured might as well update

three.js creates a WebGLState for every camera/scene combo rendered to store light and shadow data, and the three.equirectangular-to-cubemap component we use currently creates a new camera for every texture -- resulting in a new WebGLState created and retained, and does not get disposed unless calling dispose on the renderer, which will have other unintended side-effects in our case since we're not yet done with our renderer.

As a part of this change, I figured it'd be a good time to update to three's EquirectangularToCubeGenerator so we can ensure this works with the version of three we're using as a dependency (Fixes #220). Unfortunately, this generator similarly creates a new scene and camera for every texture, and not designed to handle multiple texture conversions, and the workarounds to cache these values are documented in the code.

Will file a follow up with three with changes to EquirectangularToCubeGenerator so that it can handle this scenario without creating extraneous light/shadow data, which would simplify a lot of this code when we next upgrade three.

--

Also included is a fuzzer page that, at random intervals, flips some values on the model. The element handles this surprisingly well, but a great test bed to ensure unexpected attribute combinations work well, and shake out any race conditions we have. Using memory tools, we can leave this run for quite some time and see that the memory doesn't grow.

jsantell · 2018-11-19T19:58:25Z

examples/dependencies/index.js

 import '@polymer/iron-demo-helpers/demo-snippet.js';
 import '@polymer/paper-button/paper-button.js';
+import '@polymer/paper-slider/paper-slider.js';
+import '@polymer/paper-radio-group/paper-radio-group.js';


I like using these nice components in our demos, but maybe these should be broken out per dependency as-needed in demos (e.g. only the PBR demo and fuzzer use paper-button)

There should a world where we have a build tool that figures out all of the bundle fragments across all examples without manual configuration and builds those for us (Polymer CLI does static analysis and supports configuration to do just this).

Filed #229 to track

jsantell · 2018-11-19T19:59:00Z

examples/fuzzer.html

+
+    updated.style.transition = 'none';
+    updated.style.opacity = 1;
+    requestAnimationFrame(() => {


This "update" should flash and fade on every change; doesn't work too well for lower values. Could remove

What is the value of the fade? Just ensuring that partial opacity doesn't produce unexpected side-effects?

Just to indicate to the viewer that a change occurred (more interesting for longer wait times), since sometimes the attribute changes do not result in a visible change

Going to remove this for now

jsantell · 2018-11-19T20:00:02Z

rollup.config.js

    input: './lib/model-viewer.js',
    output: {
      file: './dist/model-viewer.js',
+      sourcemap: true,


Needed for debugging memory leaks, can remove and open another issue (valuable but we're building a lot of things per npm run build currently)

WFM our users would probably appreciate the source maps being published w/ the package anyway.

jsantell · 2018-11-19T20:00:59Z

src/three-components/TextureUtils.js

+   * @return {THREE.Texture}
+   */
+  equirectangularToCubemap(texture) {
+    // Each instance of EquirectangularToCubeGenerator creates a new camera,


This special handling could go away if we make upstream fixes in three's generator

Is there an open issue against Three? Maybe link it in the comment with a note about when we can take this out?

Just filed mrdoob/three.js#15288

cdata · 2018-11-20T17:49:34Z

In terms of severity, this would only occur when constantly changing textures (not a realistic scenario IMO)

This scenario seems like it would be more common in a WYSIWYG tool that enabled GUI-based configuration of the <model-viewer>. So, probably really good to fix regardless.

cdata

Looks great, some minor changes requested

cdata · 2018-11-20T17:45:26Z

examples/dependencies/index.js

 import '@polymer/iron-demo-helpers/demo-snippet.js';
 import '@polymer/paper-button/paper-button.js';
+import '@polymer/paper-slider/paper-slider.js';
+import '@polymer/paper-radio-group/paper-radio-group.js';


There should a world where we have a build tool that figures out all of the bundle fragments across all examples without manual configuration and builds those for us (Polymer CLI does static analysis and supports configuration to do just this).

Filed #229 to track

cdata · 2018-11-20T17:46:24Z

rollup.config.js

    input: './lib/model-viewer.js',
    output: {
      file: './dist/model-viewer.js',
+      sourcemap: true,


WFM our users would probably appreciate the source maps being published w/ the package anyway.

cdata · 2018-11-20T17:52:38Z

examples/fuzzer.html

+
+    updated.style.transition = 'none';
+    updated.style.opacity = 1;
+    requestAnimationFrame(() => {


What is the value of the fade? Just ensuring that partial opacity doesn't produce unexpected side-effects?

cdata · 2018-11-20T17:54:17Z

examples/fuzzer.html

+}];
+attributes.forEach(attr => attr.values.push(null, ''));
+
+function updateAttribute (attr, values) {


I don't know how to do this in a way that won't have potentially non-relevant memory/perf side-effects, but it would helpful to log what changes happened over time somehow.

(No action required, just observing)

I was thinking of a rolling log of the last 10 changes or so when stopping, to debug a scenario where going from state X to Y causes visual artifact Z

cdata · 2018-11-20T17:56:25Z

src/third_party/three/EquirectangularToCubeGenerator.js

+
+import * as THREE from 'three';
+
+const EquirectangularToCubeGenerator = function ( sourceTexture, options ) {


Is this modified from the Three version? If so, can you call out the modifications?

Not other than the ES6 wrapper stuff, no

cdata · 2018-11-20T17:57:01Z

src/three-components/TextureUtils.js


+// Enable three's loader cache so we don't create redundant
+// Image objects to decode images fetched over the network.
+Cache.enabled = true;


cdata · 2018-11-20T18:02:28Z

src/three-components/TextureUtils.js

    this.config = {...defaultConfig, ...config};
    this.renderer = renderer;
-    this.cubemapGenerator = new EquirectangularToCubemap(this.renderer);
    this.envMapGenerator = new EnvMapGenerator(this.renderer);


Nit: initialize class field $cubeGenerator

cdata · 2018-11-20T18:03:42Z

src/three-components/TextureUtils.js

+   * @return {THREE.Texture}
+   */
+  equirectangularToCubemap(texture) {
+    // Each instance of EquirectangularToCubeGenerator creates a new camera,


Is there an open issue against Three? Maybe link it in the comment with a note about when we can take this out?

cdata · 2018-11-20T18:07:46Z

src/three-components/TextureUtils.js

+    // the prototype since this is called in the constructor.
+    if (!this[$cubeGenerator]) {
+      const material = generator.boxMesh.material;
+      EquirectangularToCubeGenerator.prototype.getShader = () => material;


Is it possible to just create an extended class rather than monkey patching on the fly here? e.g.,

class NonLeakyEquirectangularToCubeGenerator extends EquirectangularToCubeGenerator { constructor(sharedShaderMaterial = null) { super(); this.sharedShaderMaterial = sharedShaderMaterial; } getShader() { if (this.sharedShaderMaterial != null) { return this.sharedShaderMaterial; } return super.getShader(); } }

Good call, trying this

scene/camera for every texture. Add fuzzer.html example to stress test memory leaks, race conditions and uncommon attribute combinations. Switch to using three's EquirectangularToCubeGenerator, and extend with a patched version until an upstream fix in three. Fixes #220.

cdata

LGTM 👍 excited about the potential of this fuzzer

jsantell requested a review from cdata November 19, 2018 19:57

jsantell commented Nov 19, 2018

View reviewed changes

jsantell force-pushed the memory-leak branch from 726bf87 to 69c257a Compare November 19, 2018 20:05

jsantell changed the title ~~Memory leak prog~~ Fix memory leak from WebGLState light/shadow data from texture generation Nov 19, 2018

cdata suggested changes Nov 20, 2018

View reviewed changes

jsantell mentioned this pull request Nov 20, 2018

EquirectangularToCubeGenerator/PMREM* allocates and retains unnecessary WebGLStates mrdoob/three.js#15288

Closed

13 tasks

jsantell force-pushed the memory-leak branch from 69c257a to 5d0b6e5 Compare November 20, 2018 19:55

jsantell requested a review from cdata November 20, 2018 19:56

jsantell force-pushed the memory-leak branch from 5d0b6e5 to f995f77 Compare November 20, 2018 20:24

cdata approved these changes Nov 20, 2018

View reviewed changes

jsantell merged commit 5ef9706 into master Nov 20, 2018

jsantell deleted the memory-leak branch November 20, 2018 22:26

This was referenced Nov 27, 2018

(WIP) PMREM+HDR+r99 #237

Closed

Banding artifacts #239

Closed

Render bleeds to other canvases on first paint #248

Closed


		import * as THREE from 'three';

		const EquirectangularToCubeGenerator = function ( sourceTexture, options ) {

Conversation

jsantell commented Nov 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cdata commented Nov 20, 2018

Uh oh!

cdata left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cdata left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants