Improved cache control, fewer memory leaks by cdata · Pull Request #689 · google/model-viewer

cdata · 2019-08-03T00:58:27Z

This change proposes a first step towards better cache management and introduces limited cache control as a public API. Highlights include:

CacheEvictionPolicy has been introduced, to offer a consistent model for purging items from caches
A few memory leaks have been fixed:
- We were leaking all <model-viewer> elements via our integration with :focus-visible polyfill
- We have been leaking vertex buffers in a few places due to not disposing of them appropriately
CachingGLTFLoader has been integrated with CacheEvictionPolicy so that we no longer leak all of our models by design
A minor bug in cloneGltf has been fixed, which was leading us to incorrectly clone materials
A public, static, writable property modelCacheSize has been added, to enable imperative user control of GLTF caching behavior
- The value is global to all <model-viewer> instances and sets the "eviction threshold" for CachingGLTFLoader
- It can be used this way: customElements.get('model-viewer').modelCacheSize = 10;.
The Android test target has been removed, due to an undiagnosed critical failure that causes it to crash only on Sauce Labs

Future work:

Re-enable Android tests via Browserstack Re-enable Android tests via Browserstack #691

Fixes #491

elalish

Thanks for doing this! I have a few questions below...

elalish · 2019-08-05T16:56:44Z

index.html

+              <p>This static, writable property sets &lt;model-viewer&gt;'s internal
+              cache size for GLTF models. Note that any number of models may be cached
+              at any given time. This number just sets the maximum number of models
+              that can be cached even if they are not being used by a &lt;model-viewer&gt;


Reading this, I'm uncertain if this number has units of Mb or # of glTFs.

👍 will clarify

elalish · 2019-08-05T17:00:27Z

src/test/features/environment-spec.ts

+  suiteTeardown(() => {
+    // Reset the renderer once at the end of this spec, to clear out all
+    // of the heavy cached image buffers:
+    ModelViewerElementBase[$resetRenderer]();


Do we really want to destroy the gl context here? Would it be adequate to dispose of and recreate TextureUtils?

As we have discussed before, the GL context isn't especially important. What this does do is clear out a few internal Three.js caches. It's possible that we could dispose of the artifacts held in these caches more discretely, but I'm not sure how to go about doing that comprehensively.

This is only ever done once in the entire test suite (after environmennt-spec.ts is run to completion), so it shouldn't be a huge cost 🤷‍♂

elalish · 2019-08-05T17:04:14Z

src/test/three-components/TextureUtils-spec.ts

+  suiteTeardown(() => {
+    // Ensure we free up memory from loading large environment maps:
+    Cache.clear();
+    renderer.dispose();


What does disposing of the renderer do that Cache.clear() doesn't do already? Every time we recreate a renderer we get a lot of console spew that makes reading the test output harder.

See my comment above, I linked to the relevant lines. It's a hatchet, not a scalpel, for sure, but profiles bare out that there is a fair amount of memory (megabytes worth) being held by references in the renderer's internal cache.

elalish · 2019-08-05T17:12:00Z

src/three-components/CachingGLTFLoader.ts

+      this[$evictionPolicy].retain(url);
+
+      Object.defineProperty(model, $releaseFromCache, {
+        value: (() => {


This doesn't seem like our usual style; what's the purpose of Object.defineProperty here?

It's the only way I could get tsc to stop complaining about me decorating this object.

It might be possible to change the type of the identifier and get a cleaner result. I'll try that out.

elalish · 2019-08-05T17:17:23Z

src/three-components/ModelUtils.ts

  const cloneSkinnedMeshes: SkinnedMeshMap = {};

-  if (hasScene && hasSkinnedMeshes) {
+  if (hasScene) {


Can we combine this with the same if statement above?

Yeah, good call, I think we can merge the conditions.

elalish · 2019-08-05T17:21:49Z

src/three-components/TextureUtils.ts

+          environmentMap.dispose();
+        }
+      } catch (e) {
+        // Suppress errors


What errors are we getting?

If a promise is rejected (for example. say a file fails to load), the promise is rejected for all time. If you later await that promise, it will throw an error.

elalish · 2019-08-05T17:22:13Z

src/utilities.ts

 export type Constructor<T = object> = {
  new (...args: any[]): T,
-  prototype: T
+  prototype: T,


intentional?

Will remove

elalish · 2019-08-05T17:34:05Z

src/utilities/cache-eviction-policy.ts

+
+      if (retainerCount === 0) {
+        this[$cache].delete(key);
+        this[$recentlyUsed].splice(i, 1);


I don't quite follow this splice. Why are we putting a new entry into $recentlyUsed when we just deleted that entry from the cache?

The signature for JavaScript splice is:

var arrDeletedItems = array.splice(start[, deleteCount[, item1[, item2[, ...]]]])

Added items start at the third argument. So, we are only removing things here, not adding anything.

Ah, that's what I missed when I read that definition; I thought these splices were adding elements, but now I see they're removing them. I thought elements weren't being removed at all, hence my other comment. Thanks!

elalish · 2019-08-05T17:48:19Z

src/utilities/cache-eviction-policy.ts

+      this[$recentlyUsed].splice(recentlyUsedIndex, 1);
+    }
+
+    this[$recentlyUsed].unshift(key);


These two lines seem to be vaguely duplicate? Also, does the $recentlyUsed array grow without bound? Could we just make $recentlyUsed the same length as $retainerCount and just swap elements to the beginning? If only JS had a priority queue...

The JavaScript Array is unbounded and can grow indefinitely, runtime permitting. One cannot limit the size of an Array. JavaScript Array is more like a map than an actual array, where the indices are the keys. You can overload its push / pop etc. methods, but you cannot overload the index operator, and that allows you to insert to arbitrary indices (e.g., this[$recentlyUsed][49283] = true will totally do a thing here, and grow the reported length accordingly).

[$retainerCount] being a Map, it is more expensive to count the keys than to just do the book keeping directly as we retain new keys. So, surgically splicing elements out of the Array is probably the best we can do.

Got it, see above.

elalish · 2019-08-05T17:49:34Z

src/utilities/focus-visible.ts

+        disconnectedCallback() {
+          super.disconnectedCallback && super.disconnectedCallback();
+          // It's important to remove the polyfill event listener when we
+          // disconnect, otherwise we will leak the whole element via window:


Interesting!

elalish

LGTM

cdata · 2019-08-12T17:07:21Z

This change has been rebased onto master

cdata force-pushed the improved-cache-control branch 3 times, most recently from 1c22f12 to ba83a5b Compare August 3, 2019 04:44

cdata requested a review from elalish August 5, 2019 16:54

elalish reviewed Aug 5, 2019

View reviewed changes

cdata force-pushed the improved-cache-control branch from 95edcdd to b52e78b Compare August 5, 2019 19:15

cdata marked this pull request as ready for review August 5, 2019 20:52

cdata requested a review from elalish August 5, 2019 20:54

elalish previously approved these changes Aug 5, 2019

View reviewed changes

cdata dismissed elalish’s stale review via 99f520f August 12, 2019 16:44

cdata changed the base branch from newPMREM to master August 12, 2019 16:44

Chris Joel added 5 commits August 12, 2019 10:02

Improved cache control, fewer memory leaks

a832583

More memory cleanup, change Android emulator version

7fb5f27

Remove Android test target

98ae0eb

Use Map.forEach for IE11

33d25ad

IE11 means we cannot have nice things (iterators edition)

cd371ad

cdata force-pushed the improved-cache-control branch from 99f520f to cd371ad Compare August 12, 2019 17:06

cdata merged commit c5df528 into master Aug 12, 2019

cdata deleted the improved-cache-control branch August 12, 2019 17:25

cdata added this to the v0.6.0 milestone Aug 13, 2019

cdata self-assigned this Aug 26, 2019

Conversation

cdata commented Aug 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elalish 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!

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!

elalish left a comment

Choose a reason for hiding this comment

Uh oh!

cdata commented Aug 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cdata commented Aug 3, 2019 •

edited

Loading