Cesium3DTilesetStatistics improvements

[Beilinson]

Description

What I did: loaded the basic Google Photorealistic sandcastle and profiled me just moving around slowly

Performance Profile: I ordered by Self Time descending, and saw that this Cesium3DTilesetStatistics.clone takes about 8% of total frame time.

Why this change: The performance profile showed that this object destructure was taking up the main bulk of the time of this function.

I audited the code, and saw that while the statistics object is copied frame-to-frame, this property specifically is only ever used by the new statistics object, and passing it by reference kept all the same behavior in prod. Changing it a Map is also a minor performance improvement.

Additionally, I noticed that the actual amount of calls per frame to Cesium3DTilesetTraversal.updateVisibility was higher than the reported number of visited tiles, because some tiles are being updated as part of a check by their parents and are never added to the tileset traversal queue, so never marked as visited. This is rectified by explicitly visiting the tile in updateVisibility.

Current Reported Visited:

After this change:

Testing plan

Compare inspector results between main sandcastle and here, texture sizes should be the same. Visited tiles should be bigger.

main

local

Author checklist

• [x] I have submitted a Contributor License Agreement
• [x] I have added my name to CONTRIBUTORS.md
• [x] I have updated CHANGES.md with a short summary of my change
• [x] I have added or updated unit tests to ensure consistent code coverage
• [x] I have updated the inline documentation, and included code examples where relevant
• [x] I have performed a self-review of my code

[github-actions[bot]]

Thank you for the pull request, @Beilinson!

:white_check_mark: We can confirm we have a CLA on file for you.

[Beilinson]

I can fix the failing tests, but only after confirmation that the updated visitation numbers are desired. Correctness is based on what visited means:

1. Currently, whether a tile was added to the traversal queue
2. As of this PR, whether a tile was actually processed (either as part of the traversal queue or as part of a check by a parent of their children)

[lilleyse]

@Beilinson nice performance improvement!

Just a drive by comment, I haven't reviewed the code

I can fix the failing tests, but only after confirmation that the updated visitation numbers are desired. Correctness is based on what visited means:

1. Currently, whether a tile was added to the traversal queue
2. As of this PR, whether a tile was actually processed (either as part of the traversal queue or as part of a check by a parent of their children)

I would stay with option 1 - a tile is visited when it is added to the traversal queue.

[Beilinson]

Thanks @lilleyse!

I would stay with option 1 - a tile is visited when it is added to the traversal queue.

Thats fair. I worry this is minor misdirection as the count is much smaller than the true number of updated tile. Someone attempting to improve the performance of either of the traversals would be incorrect to base his performance assumptions off of that.

[Beilinson]

That's fair, just wanted to clear up that I tested this using 20x cpu slow down to simulate "high resolution" profiling since the regular performance sample rate can easily miss something like this

[Beilinson]

Just ran your sandcastle with 6x cpu slowdown, getting the same as reported in my original description:

[Beilinson]

TIL: Apparently you can set an environment variable $CPUPROFILE_FREQUENCY to control the chrome performance profiling sample rate. Default is 100. Setting this to something like 1000 and after restarting chrome I now reproduce the same results as above without any cpu slowdown:

[javagl]

Given that the texturesReferenceCounterById that this PR is about was introduced by me, I'd like to mention that I'd usually prefer a Map over a {}.

^{Usually LinkedHashMap, FWIW...}

I cannot really profoundly justify why I didn't use a Map back then. It's probably something vague, like "All the other code is using {}'s, so that's apparently how we rolll 🤷‍♂️". But the fact that most of the code uses {}'s is likely just an aspect of this being legacy code. And as one aspect of ~"trying to improve things" (even if certain improvements are happening faaar tooooo slooooowwwwlllyyyyy), I'd see this PR as a tiny improvement along these lines: It is used like a Map. Let's make it a Map.

(The fact that there are probably hundreds of other places that could be changed from {} to Map and the fact that it doesn't magically double the frame rate shouldn't prevent a clear improvement from being integrated...)

[Beilinson]

(The fact that there are probably hundreds of other places that could be changed from {} to Map and the fact that it doesn't magically double the frame rate shouldn't prevent a clear improvement from being integrated...)

I actually did some local testing by converting as many {} hash maps that I could find to a Proxy which doubles all get/set/delete operations done on the {} to a Map, and compared the performance between them. I'll hopefully get to polishing those results up tomorrow and open an issue which will give real insights into which places in the code would benefit most from getting converted to a Map, and the expected performance gains from that (spoiler, in first place is AssociativeArray)

[javagl]

The performance is one thing.

When you talk about a Proxy, then I wonder: If this is a real Proxy, wouldn't this distort the performance results considerably? It would be nice to have some automated process/tooling (maybe just IDE support) for this sort of change. Then one could take two fixed commits, once with the current state, and once with all (appropriate) objects replaced with maps. And then it would be nice if there was some sensible benchmark with which we could objectively compare the performance between these two states.

However, I assume that the performance would generally not become worse. That remains to be confirmed, but from what I've heard, a Map is generally faster for the real Map-like usage patterns.

So regardless of the performance, I think that the clarity of the code is really important. First of all, I don't like the untypedness of JavaScript to begin with. The fact that you can just takeSomeObject.andAssignArbitraryProperties is dangerous for the maintainability in a large codebase. On top of that, is is practically impossible to find out what a line of code like object.x = 42; is doing. (Yeah, that sounds strange, but you never know whether that's a 'setter' - so... just do a full-text-search for x, right?) For the specific case of the Map-like usage pattern, there now are the Object.keys and Object.values functions, but the whole codebase is still littered with these old something.hasOwnProperty(property) calls.

---

BTW:

spoiler, in first place is AssociativeArray

It was exactly two weeks ago (October 1st ) when I asked in our internal chat...

Any reason to not "replace all" of https://github.com/CesiumGS/cesium/blob/ecf0050a7b185fc9dd4a6b85ab6e0453ec89f349/packages/engine/Source/Core/AssociativeArray.js with Map?

---

The bottom line: Map where a Map is due 🙂

[ggetz]

Also a drive by comment: I don't think at this point there's reason not to move to Map other than the time to, like @javagl said, make sure where we do swap them in is the right compromise between performance and clarity.

For AssocitaiveArray specifically, it is part of the public API so, if we decide to remove it, we should properly deprecate first.

Though there's no reason to prevent this PR from going in with just this one Map update as it has a demonstrated benefit.

[ggetz]

The fact that you can just takeSomeObject.andAssignArbitraryProperties is dangerous for the maintainability in a large codebase.

And I believe doing so also can have performance implications if "dictionary mode" is triggered.

[jjhembd]

real insights into which places in the code would benefit most from getting converted to a Map, and the expected performance gains from that (spoiler, in first place is AssociativeArray)

@Beilinson I've had a long-running note to myself to get rid of AssociativeArray. My only reasoning was that it was an unnecessary duplication of Map. But if you also have performance data to justify the change, that's even better!

As @ggetz mentioned, we do have to communicate any change clearly, since it's public. Maybe replacing AssociativeArray should be a separate issue/PR by itself.

[Beilinson]

Happy to get conversation (re)started about this! AssociativeArray specifically should definitely be a separate issue (I know my org has code that uses the public API surface for example viewer.entities which is an AssociativeArray). I'd love to continue this in the separate issue I'll open.

Is this PR ready to merge then?

[jjspace]

Is this PR ready to merge then?

We definitely got a little off track. All good discussion but it can be moved into that issue you just opened. I agree with @ggetz that there's no reason to hold up this specific switch to Map. I do still have one concern, posted above, that got lost earlier