Skip to content

Cesium3DTilesetStatistics improvements #12974

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
+43 −16
Open

Cesium3DTilesetStatistics improvements #12974

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
dd82a80
Remove Cesium3DTilesetStatistics.textureReferenceCounterById destructure
Beilinson Oct 7, 2025
1f2458c
tileset traversal visit fix
Beilinson Oct 10, 2025
729a10b
Revert "tileset traversal visit fix"
Beilinson Oct 10, 2025
5150e74
Changelog
Beilinson Oct 10, 2025
137d023
Merge branch 'main' into 3d-tileset-statistics-performance
Beilinson Oct 13, 2025
f77f097
Merge branch 'main' into 3d-tileset-statistics-performance
Beilinson Oct 14, 2025
2f5822b
Snapshot example
Beilinson Oct 16, 2025
617a9e5
Fix undefined statisticsLast
Beilinson Oct 16, 2025
c1b4757
Merge branch 'main' into 3d-tileset-statistics-performance
Beilinson Nov 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
- Improved performance of `scene.drillPick`. [#12916](https://github.com/CesiumGS/cesium/pull/12916)
- Improved performance when removing primitives. [#3018](https://github.com/CesiumGS/cesium/pull/3018)
- Improved performance of terrain Quadtree handling of custom data [#12907](https://github.com/CesiumGS/cesium/pull/12907)
- Improved rendering performance when a 3D tileset is loaded [#12974](https://github.com/CesiumGS/cesium/pull/12974)

## 1.134.1 - 2025-10-10

Expand Down
16 changes: 7 additions & 9 deletions packages/engine/Source/Scene/Cesium3DTilesetStatistics.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ function Cesium3DTilesetStatistics() {
// Memory statistics
this.geometryByteLength = 0;
this.texturesByteLength = 0;
this.texturesReferenceCounterById = {};
this.texturesReferenceCounterById = new Map();
this.batchTableByteLength = 0; // batch textures and any binary metadata properties not otherwise accounted for
}

Expand Down Expand Up @@ -100,12 +100,12 @@ Cesium3DTilesetStatistics.prototype.incrementLoadCounts = function (content) {
const textureIds = content.getTextureIds();
for (const textureId of textureIds) {
const referenceCounter =
this.texturesReferenceCounterById[textureId] ?? 0;
this.texturesReferenceCounterById.get(textureId) ?? 0;
if (referenceCounter === 0) {
const textureByteLength = content.getTextureByteLengthById(textureId);
this.texturesByteLength += textureByteLength;
}
this.texturesReferenceCounterById[textureId] = referenceCounter + 1;
this.texturesReferenceCounterById.set(textureId, referenceCounter + 1);
}
}

Expand Down Expand Up @@ -146,13 +146,13 @@ Cesium3DTilesetStatistics.prototype.decrementLoadCounts = function (content) {
// total textures byte length
const textureIds = content.getTextureIds();
for (const textureId of textureIds) {
const referenceCounter = this.texturesReferenceCounterById[textureId];
const referenceCounter = this.texturesReferenceCounterById.get(textureId);
if (referenceCounter === 1) {
delete this.texturesReferenceCounterById[textureId];
this.texturesReferenceCounterById.delete(textureId);
const textureByteLength = content.getTextureByteLengthById(textureId);
this.texturesByteLength -= textureByteLength;
} else {
this.texturesReferenceCounterById[textureId] = referenceCounter - 1;
this.texturesReferenceCounterById.set(textureId, referenceCounter - 1);
}
}
}
Expand Down Expand Up @@ -187,9 +187,7 @@ Cesium3DTilesetStatistics.clone = function (statistics, result) {
statistics.numberOfTilesCulledWithChildrenUnion;
result.geometryByteLength = statistics.geometryByteLength;
result.texturesByteLength = statistics.texturesByteLength;
result.texturesReferenceCounterById = {
...statistics.texturesReferenceCounterById,
};
result.texturesReferenceCounterById = statistics.texturesReferenceCounterById;
Copy link
Contributor

@jjspace jjspace Oct 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems Github ate this comment and didn't post it with my original review. This is my main remaining concern with this PR that it's no longer a full clone

What do you think about cloning the map here instead of just passing by reference? In some limited testing it seemed like it was still a big improvement over the spread ... but also stays more true to the meaning of clone by actually cloning?

Suggested change
result.texturesReferenceCounterById = statistics.texturesReferenceCounterById;
result.texturesReferenceCounterById = new Map(statistics.texturesReferenceCounterById);

Copy link
Contributor Author

@Beilinson Beilinson Oct 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just compared all three options using your sandbox and CPUPROFILE_FREQUENCY=1000, the results:

Before this pr:
image

Clone with new Map (this suggestion):
image

No deep clone (my version):
Don't have anything to show here because it doesn't show up on the performance sample

This change makes it no longer a "deep clone", it still is a "shallow clone", and I dont think the deep behavior is needed for a completely internal part of this class which has zero effect on any part of the external runtime or the end result when comparing statistics before/after rendering. IMO, even if its only 4% of total cpu time, thats still 4% that has no added value on the outcome of the statistics counting.

This element seems to only be used by the new version (the clone) and never the old object, the actual texturesByteLength which this is attribute is used to update is copied over.

For context, at 4% this unnecessary clone takes more time than Matrix4.multiplyTransformation or binding the vertices to webgl:

image

4% rendertime isn't critical, but I think if many of these small percentage improvements can be made that will make a big long-term difference.

Thoughts?

Copy link
Contributor

@javagl javagl Oct 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jjspace You mentioned the cloning aspect in the now-closed PR at #12968 (comment) (but that referred to the credits, so I'm not sure if this is what you meant...?)

I'm also in strong favor of avoiding surprises: clone should clone (no shallow copy with unpredictable side-effects).

But... I had a short look at where Cesium3DTilesetStatistics.clone is actually called and how it is used.

One call is in update (every call is in some update 😆 ). It's not really documented what's happening there. But it seems to fill some tileset._statisticsPerPass array. This array, in turn, does not seem to be used anywhere.

The other call is in raiseLoadProgressEvent. There, it fills some tileset._statisticsLast. But after that, it only seems to care about the numberOfPendingRequests and numberOfTilesProcessing there (and certainly not about that map with the texture IDs).

Sooo... unless I'm overlooking something, the places that are using that clone function are doing a lot of unnecessary work to begin with, and on top of that, none of them seems to depend on that Map. I'm sure there is some room for improvement.

One point that might be relevant for real peformance comparisons: What's in that Map after all? The performance will to some extent depend on the size of that map. On the other hand: When there are 10 textures, then the map is small, and when there are 1000 textures, then there is other costly stuff - so I'd expect this to not be a bottleneck either way.

Copy link
Contributor Author

@Beilinson Beilinson Oct 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@javagl The map gets pretty big (hundreds of entries) of [string-number] pairs.

I checked the instances of the statistics cloning and saw the same as you reported. Either way, the clone is used more as a "Lets get a snapshot of what the statistics currently looks like", wherein the map is not useful.

_statisticsPerPass is used in the specs, and also in the Cesium3DTilesInspectorViewModel.getStatistics. Here all the internal properties are needed except the Map again, because these act as snapshots of what the statistics looked like while processing the pick/render/whatever pass, and aren't actively used for calculating anything.

Ideas:

  1. Make this a separate function (.snapshot or open to other suggestions) that only passes around these values without copying the map. It would return an object that doesn't have the functions to increment/decrement the values, representing that its a static snapshot object that shouldn't be worked on directly.
  2. Explicitly copy over only what is needed, so _statisticsLast would become an object containing only numberOfPendingRequests and numberOTilesProcessing, and the passes would copy over everything except the map. I think the snapshot model is cleaner.
    thoughts @javagl @jjspace ?

Copy link
Contributor Author

@Beilinson Beilinson Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed an example of what the snapshot would look like, note that the passes and last statistics now are no longer full statistics objects, so they dont contain functions to increment/decrement counts or any other logic, just container objects. They also don't contain the map.

Copy link
Contributor

@javagl javagl Oct 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong opinion, but a few random comments for now:

  • When something is only used in tests, it's a hint that it may be removed
  • I didn't have the Cesium3DTilesInspectorViewModel on the radar (usually only searching in ./packages/engine/Source...)
  • The whole structure and role of the statistics raises a bunch of questions
    • Top-level ones: Which of them are purely informative (to be shown in a UI), and which of them are crucially influencing the behavior of the whole application? I think that the _statisticsPerPass could/should carry a comment saying that it's only for the UI. And for me, the inlined "block comments", // Rendering statistics, // Loading statistics ... etc. are screaming that there should be RenderingStatistics and LoadingStatistics sub-structures.

The 'snapshot' approach looks conceptually clean for me, but implies new structures (first and foremost that new type, ...Snapshot) and several changes that may warrant some explaining (maybe even just /** A snapshot is a shallow copy ... */ or so).

If this was only about the raiseLoadProgressEvent, I could imagine that storing both relevant fields explicitly, as
tileset._lastNumberOfPendingRequests and _lastNumberOTilesProcessing could be an option a well. (Yes, this "litters" the tileset even more, but ... now there's that statisticsLast which carries mostly unused stuff, so both are not ideal anyhow...)

Copy link
Contributor Author

@Beilinson Beilinson Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree about statisticsLast (although future logic may want to use the other attributes for certain logic maybe).

But yes it's both used for specs (which in my opinion would preferably be something hidden by a debug flag) but also for the inspector view to debug either the pick/render pass (from what I saw nearly every member other than the map is used).

I would prefer not to stretch the structural changes in this PR much more than I have already, but let me know what you think is best, just comment the areas for future reference or something different

Copy link
Contributor

@javagl javagl Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, no strong opinion (I assume others will chime in here), but

  • Change to Map: Good
  • Shallow copy instead of clone? Confusing
  • The changes from the last commit ("snapshot") go a bit too far and raise too many questions for me

A compromise:

Iff the performance benefit is really worth it, maybe just renaming the clone to shallowCopy could be reasonable? (It's an undocumented/internal function after all.. )

Copy link
Contributor Author

@Beilinson Beilinson Oct 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Im fine with that, shallowClone + comment explaining that the clones should not be worked on directly sounds good to me.

I could also set the map to undefined so if in the future someone does try to use one of the clones it shoulderror out.

thoughts @jjspace /and or others?

result.batchTableByteLength = statistics.batchTableByteLength;
};
export default Cesium3DTilesetStatistics;