search

CesiumGS / cesium-native

Apache License 2.0
438 stars 215 forks source link

Cache images across glTFs to avoid duplication #926

Closed azrogers closed 3 weeks ago

azrogers commented 4 months ago

As described in #497, sometimes tilesets contain multiple tiles that point to the same image resources. At the moment, these images are loaded once for every tile that uses them, meaning a tileset with 1,000 tiles that use the same image will load that image 1,000 times (and the runtimes will allocate video memory for each of them). This change adds a SharedAssetDepot class that stores images across tiles, and a SharedAsset smart pointer type for tracking and cleaning up the images.

SharedAssetDepot is set up so that in the future, if we need, we can extend this feature to handle glTF buffers as well.

There's a few TODO items currently (besides the runtime implementations):

lilleyse commented 3 months ago

@azrogers do you think it will be straightforward to extend this approach to also support external schemas (#727) in a follow-up PR?

azrogers commented 3 months ago

@lilleyse I've designed the SharedAssetDepot to support multiple kinds of assets, though only images are implemented in this PR. It should support extending it to also handle loading external schemas.

javagl commented 2 months ago

Every copy of the image is still counted towards memory used, making the metric inaccurate.

Even though it is not really "relevant", I cannot help but draw the connection to https://github.com/CesiumGS/cesium/issues/11897 (Sooo... at least we have consistency 😶 )

kring commented 1 month ago

I'm thinking about how the shared image cache should work. As implemented so far, it's time based. When a potentially-shared image becomes unused, it's added to a list of deletion candidates. The deletion candidates are allowed to age for 60 seconds, and then they're deleted. (Aside: I don't see any code currently to remove a shared image from the deletionCandidates if it becomes used again before it's evicted entirely.)

This requires a bit of bookkeeping every frame to age the deletion candidates, but the bigger problem is that 60 seconds is a long time. On a fast connection (or when loading from the request cache), we could end up loading a massive amount of data in that time, and keeping it resident unnecessarily. This probably isn't likely to be a huge problem in the normal use-cases for which this feature was implemented, but consider a pathological tileset where every glTF has a separate image loaded from a separate external URL. Or if we start using this same system for raster overlay quadtree sub-tiles as mentioned in #958. We could end up with some very high memory usage.

I think it would be better to limit the cache by size instead. When an image becomes unused, it gets added to the end of the deletion queue and the total size of the deletion queue is increased accordingly. When the total size of the unused images is above a user-configurable limit, the images at the front of the queue are deleted until we're below the limit again. If an image becomes used again while it's already in the deletion queue, be sure to remove it from the deletion queue.

This pattern should require a bit less bookkeeping, and should keep memory usage more stable.

I also would really like to see if it's possible to remove the AssetContainer and SharedAsset types, and have the AssetDepot hold reference-counted ImageCesium instances directly. I find the relationship between these types quite confusing (even after I tried to draw myself a diagram). For one thing, I can't quite understand why AssetContainer has two reference counts: one in its base class CesiumUtility::ReferenceCounted, and one in a field called counter. I think there might be a lot more complexity here than there needs to be. It would be fine to add an assetId to ImageCesium

I'm happy to take a crack at these two if that sounds good to you, @azrogers. Or if you want to work on it, perhaps after we've had a chance to discuss, then that's fine too!

javagl commented 1 month ago

This comment is dark 😈

But more seriously, and without having looked at the details of the implementation: The "eviction strategy" - i.e. whether it is time-based or size-based - might lend itself to be a virtual class EvictionStrategy. There could then be two implementations (size and time), or a "combined" strategy. And I'd strongly suggest to at least consider the common, well-known eviction strategies (Least Recently Used, Least Frequently Used, ... or maybe something like "evict largest entries first"). Each of them may have pros and cons for the respective combination of "tileset structure" and "usage pattern". But it sounds like an important point to "hook in" some abstraction, to quickly try out different strategies.

azrogers commented 1 month ago

Trying to slim down the number of open PRs we have on this. Two remaining bullet points from @kring to cover:

azrogers commented 1 month ago

Made the two changes I mentioned in standup, passing the AssetFactory to the SharedAssetDepot through the constructor instead of through the getOrFetch call, and changing the deletionCandidates list to a DoublyLinkedList. Going to investigate getting it building with Unity next.

azrogers commented 1 month ago

Changes to get Unity building again are now in CesiumGS/cesium-unity#518. These changes don't implement the shared asset depot, they only get it back to the current level of functionality before the cesium-native shared asset changes.

azrogers commented 3 weeks ago

@kring Reviewed your changes to cesium-native and cesium-unreal. Both look good, only found a minor typo to fix (which I did myself). Looks like getting all the CI checks to pass is the next step here - anything else remaining before we can get this in?

kring commented 3 weeks ago

Thanks @azrogers, I plan to merge this today!

kring commented 3 weeks ago

This looks great, merging!