Skip to content
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.

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

Add memory statistics for the global ResourceCache #10413

Merged
merged 21 commits into from
Jun 8, 2022

Conversation

ptrgags
Copy link
Contributor

@ptrgags ptrgags commented May 31, 2022

Part of #9886, this PR tracks memory usage in the global ResourceCache used to load resources for ModelExperimental. It is the global counterpart to #10397 which tracks memory on a per-ModelExperimental basis.

Some important caveats:

  • This only measures memory loaded through the cache, not all global memory usage in general.
  • There is overlap between the memory counted here and the memory counted in Add basic memory statistics in ModelExperimental #10397. Memory can be shared between multiple tilesets due to the global cache. Meanwhile, a ModelExperimental may use a mix of resources loaded from the cache, and additional buffers generated at runtime as needed. Determining the size of the overlap is non-trivial.

Sandcastle for testing -- In the Logging section of the inspector, toggle on the new ResourceCacheStatistics like so:
image

@j9liu could you review?

CC @lilleyse

@ptrgags ptrgags requested a review from j9liu May 31, 2022 18:07
@cesium-concierge
Copy link

Thanks for the pull request @ptrgags!

  • ✔️ 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.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.

Copy link
Contributor

@j9liu j9liu left a comment

Choose a reason for hiding this comment

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

@ptrgags did a first pass and left some comments!

Source/Scene/ResourceCache.js Outdated Show resolved Hide resolved
Source/Scene/ResourceCacheStatistics.js Outdated Show resolved Hide resolved
Source/Scene/ResourceCacheStatistics.js Show resolved Hide resolved
Source/Scene/ResourceCacheStatistics.js Outdated Show resolved Hide resolved
Source/Scene/ResourceCacheStatistics.js Outdated Show resolved Hide resolved
Specs/Scene/ResourceCacheStatisticsSpec.js Show resolved Hide resolved
Source/Scene/ResourceCache.js Show resolved Hide resolved
Specs/Scene/ResourceCacheSpec.js Show resolved Hide resolved
@ptrgags
Copy link
Contributor Author

ptrgags commented Jun 7, 2022

Just leaving another Sandcastle link for testing property tables in the inspector, still have to fix the missing unit tests

@ptrgags
Copy link
Contributor Author

ptrgags commented Jun 7, 2022

@j9liu updated!

Copy link
Contributor

@j9liu j9liu left a comment

Choose a reason for hiding this comment

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

@ptrgags -- did another pass. The biggest glaring issue is that the dropdown in the inspector isn't formatted well... maybe something additional needs to be modified in the HTML?

Also this PR should get logged in CHANGES.md.

@ptrgags
Copy link
Contributor Author

ptrgags commented Jun 8, 2022

@j9liu updated!

@j9liu
Copy link
Contributor

j9liu commented Jun 8, 2022

@ptrgags you missed one of my comments; there's a unit test in ResourceCacheSpec, "loads index buffer from accessor as buffer", where the statistics aren't counted at the end of the unit test.

EDIT: also CHANGES.md

@ptrgags
Copy link
Contributor Author

ptrgags commented Jun 8, 2022

@j9liu while trying to fix the broken test, I learned that the metadata buffer views are copied and unloaded from the cache. The overall StructuralMetadataLoader is not cached, so really it shouldn't have been counted after all. I'll work on removing it from the count

@ptrgags
Copy link
Contributor Author

ptrgags commented Jun 8, 2022

@j9liu updated!

@j9liu
Copy link
Contributor

j9liu commented Jun 8, 2022

@ptrgags - I just fixed a typo in the changelog, otherwise this looks good. I'll wait on Travis to pass before I merge.

@j9liu j9liu merged commit b6cf89e into main Jun 8, 2022
@j9liu j9liu deleted the resource-cache-statistics branch June 8, 2022 21:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants