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

QuadtreeRasterOverlay sub-tile cache size accounting is wrong #958

Open
kring opened this issue Oct 3, 2024 · 1 comment
Open

QuadtreeRasterOverlay sub-tile cache size accounting is wrong #958

kring opened this issue Oct 3, 2024 · 1 comment
Labels
bug Something isn't working performance Improvements to performance, including reductions in memory usage

Comments

@kring
Copy link
Member

kring commented Oct 3, 2024

QuadtreeRasterOverlay has a small cache that is intended to briefly store decoded raster overlay tile images that span multiple geometry tiles, so we don't need to decode them multiple times (or download them multiple times in the hopefully-unlikely event that the requests aren't cached).

But it doesn't appear to be working correctly.

QuadtreeRasterOverlayTileProvider has a _cachedBytes field. When a new overlay image is loaded, this field is immediately incremented accordingly. The code that later decrements this field looks like this:

this->_tileLookup.erase(it->tileID);
it = this->_tilesOldToRecent.erase(it);

// If this is the last use of this data, it will be freed when the shared
// pointer goes out of scope, so reduce the cachedBytes accordingly.
if (pImage.use_count() == 1) {
  if (pImage->image) {
    this->_cachedBytes -= int64_t(pImage->image->pixelData.size());
    CESIUM_ASSERT(this->_cachedBytes >= 0);
  }
}

This snippet is executed when we're removing this tile from the cache because the cache is full and this tile is the one that is least-recently-used. It is not necessarily true that this image is unused when this happens. And because of that use_count() == 1 check, if this tile happens to be in use elsewhere when it's evicted from the cache, we'll never subtract its bytes.

As a result, over time (and it doesn't take very long), we'll think the cache is completely full at all times and each new image will be evicted immediately, making the cache pretty close to useless.

I think the solution is simply to remove that use_count() == 1 check. When we evict an image from the cache, we should always reduce the number of bytes in the cache accordingly. Based on the comment, when I wrote this, I seemed to be hung up on the question of whether the image would actually be freed. But the freeing doesn't matter; all that matters is whether this image is kept in the set of tiles temporarily kept around even though they're potentially unused.

@kring kring added bug Something isn't working performance Improvements to performance, including reductions in memory usage labels Oct 3, 2024
@kring
Copy link
Member Author

kring commented Oct 3, 2024

This entire sub-tile cache should probably just be replaced by the external image cache in #926!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working performance Improvements to performance, including reductions in memory usage
Projects
None yet
Development

No branches or pull requests

1 participant