Enforce memory limit for `Cesium3DTilesetCache` #11310

jjhembd · 2023-05-25T16:51:57Z

Resolves #6226.

This PR deprecates the Cesium3DTileset constructor option maximumMemoryUsage, and replaces it with two new options:

cacheBytes: The size (in bytes) to which the tile cache will be trimmed, if the cache contains tiles not needed for the current view.
maximumCacheOverflowBytes: The maximum additional memory (in bytes) to allow for cache overflow, if more than cacheBytes are needed for the current view.

If the tileset's totalMemoryUsageInBytes exceeds cacheBytes + maximumCacheOverflowBytes, processing of new tiles will be interrupted, and the target screen-space error used to drive level-of-detail refinement will be adjusted higher. When the camera moves to an area where totalMemoryUsageInBytes drops below cacheBytes, the target screen-space error will be adjusted back down toward the user-provided value in maximumScreenSpaceError.

The adjusted screen-space error is exposed as a new read-only property memoryAdjustedScreenSpaceError

Here is a hosted Sandcastle demonstrating the interaction between memory usage and adjusted screen-space error.

cesium-concierge · 2023-05-25T16:52:01Z

Thanks for the pull request @jjhembd!

✔️ Signed CLA found.
❔ CHANGES.md was not updated.
- If this change updates the public API in any way, please add a bullet point to CHANGES.md.
❔ Unit tests were not updated.
- Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

Cesium Viewer works.
Works in 2D/CV.

ggetz · 2023-05-26T19:59:33Z

Instead of cacheHeadroomBytes, perhaps maximumCacheOverflowBytes?

packages/engine/Source/Scene/Cesium3DTileset.js

Remove console.logs

lilleyse · 2023-05-30T18:39:37Z

@jjhembd if it's not too much trouble would it be possible to move the cleanup changes into a separate PR? I have some minor comments there but don't want to distract from the main review.

jjhembd · 2023-05-30T21:01:34Z

would it be possible to move the cleanup changes into a separate PR?

@lilleyse I moved all the cleanup commits into the texture-cleanup branch. The changes here should now be only the memory limit changes.

packages/engine/Source/Scene/Cesium3DTile.js

lilleyse · 2023-06-05T18:23:53Z

packages/engine/Source/Scene/Cesium3DTileset.js

+ * @property {number} [maximumMemoryUsage=512] The maximum amount of memory in MB that can be used by the tileset. Deprecated.
+ * @property {number} [cacheBytes=536870912] The size (in bytes) to which the tile cache will be trimmed, if the cache contains tiles not needed for the current view.
+ * @property {number} [maximumCacheOverflowBytes=536870912] The maximum additional memory (in bytes) to allow for cache headroom, if more than {@link Cesium3DTileset#cacheBytes} are needed for the current view. Must be at least 10 * 1024 * 1024.


Make sure to include instructions in CHANGES.md for replicating the previous behavior.

I think that would be setting maximumCacheOverflowBytes to Number.MAX_VALUE?

lilleyse · 2023-06-05T18:25:55Z

packages/engine/Source/Scene/Cesium3DTileset.js

+ * @property {number} [maximumMemoryUsage=512] The maximum amount of memory in MB that can be used by the tileset. Deprecated.
+ * @property {number} [cacheBytes=536870912] The size (in bytes) to which the tile cache will be trimmed, if the cache contains tiles not needed for the current view.
+ * @property {number} [maximumCacheOverflowBytes=536870912] The maximum additional memory (in bytes) to allow for cache headroom, if more than {@link Cesium3DTileset#cacheBytes} are needed for the current view. Must be at least 10 * 1024 * 1024.


Must be at least 10 * 1024 * 1024

This seems a bit arbitrary. Why can't it be 0?

We currently allow maximumMemoryUsage == 0, to force the cache to be trimmed to the minimum size possible. This is used several times in the specs, so I also allowed cacheBytes == 0 for consistency.

If both cacheBytes == 0 and maximumCacheOverflowBytes == 0, then no tiles will be processed and memoryAdjustedScreenSpaceError will grow to infinity. This feels buggy, as Matt noticed already, even with a small non-zero value.

I chose 10 MB as a number that should guarantee we render at least one tile.

As discussed with @ggetz, I changed this to allow maximumCacheOverflowBytes == 0, and simply added a oneTimeWarning inside increaseScreenSpaceError, to alert the developer that the values of cacheBytes and maximumCacheOverflowBytes might be too small.

lilleyse · 2023-06-05T18:27:33Z

packages/engine/Source/Scene/Cesium3DTileset.js

+   * If tiles sized more than <code>cacheBytes</code> are needed to meet the
+   * desired screen space error, determined by {@link Cesium3DTileset#maximumScreenSpaceError},
+   * for the current view, then the memory usage of the tiles loaded will exceed
+   * <code>cacheBytes</code>.  For example, if the maximum is 256 MB, but 300 MB
+   * of tiles are needed to meet the screen space error, then 300 MB of tiles may
+   * be loaded. When these tiles go out of view, they will be unloaded.


This description is a bit incomplete without also mentioning maximumCacheOverflowBytes

lilleyse · 2023-06-05T18:29:41Z

packages/engine/Source/Scene/Cesium3DTileset.js

@@ -1545,17 +1571,123 @@ Object.defineProperties(Cesium3DTileset.prototype, {
   *
   * @exception {DeveloperError} <code>maximumMemoryUsage</code> must be greater than or equal to zero.
   * @see Cesium3DTileset#totalMemoryUsageInBytes
+   *
+   * @deprecated
   */
  maximumMemoryUsage: {


Several tests should be updated to use cacheBytes.

lilleyse · 2023-06-05T18:31:37Z

packages/engine/Source/Scene/Cesium3DTileset.js

+  const tiles = tileset._processingQueue.filter(
+    (tile) =>
+      !tile.isDestroyed() &&
+      tile._contentState === Cesium3DTileContentState.PROCESSING
+  );


While more concise, this will allocate an array every frame (which is what filterProcessingQueue avoids)

I restored filterProcessingQueue for now.

We are also potentially re-allocating by using tileset._processingQueue.push() in requestContent. If this is really an issue, perhaps a separate PR could convert ._processingQueue to a ManagedArray, and move the custom filter function to a method of ManagedArray.

lilleyse · 2023-06-05T18:43:44Z

packages/engine/Source/Scene/Cesium3DTileset.js

+  for (let i = 0; i < tiles.length; ++i) {
+    tiles[i].updatePriority();
  }
+  tiles.sort(sortTilesByPriority);


I wonder if we should always be updating priority and sorting, not just when the SSE changes. Because it doesn't seem strictly necessary to just do it here.

Also is there a reason it's updated here and not in decreaseScreenSpaceError?

Increasing screen space error may result in some of the tiles in ._processingQueue becoming irrelevant for rendering. This is why I thought a sort was essential here, and not in decreaseScreenSpaceError.

I at first tried sorting every frame (in processTiles), but couldn't see any performance benefit. So I avoided too many sorts to be safe. Note that ._processingQueue can be significantly longer than ._requestedTiles, so this sort is potentially more expensive.

lilleyse · 2023-06-05T18:47:24Z

packages/engine/Source/Scene/Cesium3DTileset.js

Reminder to add some unit tests

jjhembd · 2023-06-07T17:11:35Z

Thanks @lilleyse! I think I've addressed all your feedback.

@ggetz can you give this a final look?

ggetz

Thanks @jjhembd! This is working well! Just a few comments.

ggetz · 2023-06-07T17:54:43Z

CHANGES.md

+
+##### Additions :tada:
+
+- Added `Cesium3DTileset.cacheBytes` and `Cesium3DTileset.maximumCacheOverflowBytes` to better control memory usage. To replicate previous behavior, change `maximumMemoryUsage` to `cacheBytes`, and set `maximumCacheOverflowBytes = Number.MAX_VALUE`


And convert MB to bytes, correct?

CHANGES.md

packages/engine/Source/Scene/Cesium3DTileset.js

ggetz · 2023-06-07T18:06:26Z

packages/engine/Specs/Scene/Cesium3DTilesetSpec.js

@@ -3471,11 +3471,11 @@ describe(
      });
    });

-    it("Unload some cached tiles not required to meet SSE using maximumMemoryUsage", function () {
+    it("Unload some cached tiles not required to meet SSE using cacheBytes", function () {


Since it was deprecated, can we leave the old tests specifically for maximumMemoryUsage, and add new ones to cover cacheBytes?

jjhembd · 2023-06-07T20:25:23Z

Thanks @ggetz! I think I addressed all your feedback. Let me know if there's anything else.

ggetz · 2023-06-07T20:33:31Z

All looks good to me! Thanks @jjhembd!

jjhembd added category - 3d tiles category - memory/performance labels May 25, 2023

jjhembd marked this pull request as ready for review May 26, 2023 16:24

jjhembd requested a review from lilleyse May 26, 2023 17:05

ggetz reviewed May 26, 2023

View reviewed changes

packages/engine/Source/Scene/Cesium3DTileset.js Outdated Show resolved Hide resolved

Jeshurun Hembd added 9 commits May 29, 2023 19:49

Adjust tileset screen space error if out of memory

0fa7440

Sort tileset processing queue when hitting memory limit

c93423a

Add cachedBytes and maximumCachedBytes tileset options

860dfb3

Fix failing tests and types

92d7493

Remove console.logs

Use cacheHeadroomBytes to specify memory limit for tileset cache

703a39f

Tweak memory-adjusted screen space error functions

a8ddd97

Use larger cache size in createGooglePhotorealistic3DTileset

4e8006e

Require cacheHeadroomBytes to be at least 10 MB

558fcdb

Rename cacheHeadroomBytes to maximumCacheOverflowBytes

9c3e810

jjhembd force-pushed the tileset-cache branch from 943a5f4 to 9c3e810 Compare May 30, 2023 20:57

lilleyse reviewed Jun 5, 2023

View reviewed changes

Jeshurun Hembd added 4 commits June 5, 2023 19:01

First round PR feedback

57fe792

Merge branch 'main' into tileset-cache

4087cd4

Update PointCloudStylingPipelineStageSpec

9eff548

Allow maximumCacheOverflowBytes == 0, and warn if too small

4e99c91

Second round PR feedback

f1baff4

ggetz reviewed Jun 7, 2023

View reviewed changes

jjhembd mentioned this pull request Jun 7, 2023

Remove Cesium3DTileset.maximumMemoryUsage #11344

Closed

ggetz merged commit 5de34c2 into main Jun 7, 2023

ggetz deleted the tileset-cache branch June 7, 2023 20:33

jjhembd mentioned this pull request Aug 2, 2023

Race condition in Cesium3DTileset.memoryAdjustedScreenSpaceError #11447

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enforce memory limit for `Cesium3DTilesetCache` #11310

Enforce memory limit for `Cesium3DTilesetCache` #11310

jjhembd commented May 25, 2023 •

edited

Loading

cesium-concierge commented May 25, 2023

ggetz commented May 26, 2023

lilleyse commented May 30, 2023

jjhembd commented May 30, 2023

lilleyse Jun 5, 2023

lilleyse Jun 5, 2023

jjhembd Jun 5, 2023

jjhembd Jun 7, 2023

lilleyse Jun 5, 2023

lilleyse Jun 5, 2023

lilleyse Jun 5, 2023

jjhembd Jun 5, 2023

lilleyse Jun 5, 2023

jjhembd Jun 5, 2023

lilleyse Jun 5, 2023

jjhembd commented Jun 7, 2023

ggetz left a comment

ggetz Jun 7, 2023

ggetz Jun 7, 2023

jjhembd commented Jun 7, 2023

ggetz commented Jun 7, 2023


		##### Additions :tada:

		- Added `Cesium3DTileset.cacheBytes` and `Cesium3DTileset.maximumCacheOverflowBytes` to better control memory usage. To replicate previous behavior, change `maximumMemoryUsage` to `cacheBytes`, and set `maximumCacheOverflowBytes = Number.MAX_VALUE`

Enforce memory limit for Cesium3DTilesetCache #11310

Enforce memory limit for Cesium3DTilesetCache #11310

Conversation

jjhembd commented May 25, 2023 • edited Loading

cesium-concierge commented May 25, 2023

ggetz commented May 26, 2023

lilleyse commented May 30, 2023

jjhembd commented May 30, 2023

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

jjhembd commented Jun 7, 2023

ggetz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjhembd commented Jun 7, 2023

ggetz commented Jun 7, 2023

Enforce memory limit for `Cesium3DTilesetCache` #11310

Enforce memory limit for `Cesium3DTilesetCache` #11310

jjhembd commented May 25, 2023 •

edited

Loading