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

Fix memory cache #762

Merged
merged 14 commits into from
Sep 30, 2024
Merged

Fix memory cache #762

merged 14 commits into from
Sep 30, 2024

Conversation

gkjohnson
Copy link
Contributor

@gkjohnson gkjohnson commented Sep 25, 2024

Fix #761

@gkjohnson
Copy link
Contributor Author

cc @AnthonyGlt - this should be ready if you want to test it a bit. I just have to write some more tests / fix the existing ones.

In simpler terms, the changes are:

  • Split the "max size" eviction and the "min size" eviction while loops so the logic is easier to follow.
  • Add a concept of "fully loaded" items. If an item isn't full loaded then it can be evicted from the cache if it's above the "max" size even if its marked as "used". This lets us stop any downloading or yet-to-be-parsed tile geometry if the cache has suddenly become full.
  • The loading logic now checks the if the geometry memory usage has been assigned before determining if tile data should be evicted after loading.

@gkjohnson
Copy link
Contributor Author

PR should be ready, now.

Comment on lines 149 to 150
// loaded are subject to being disposed early even if the are marked as used if the cache is full above its
// max size limits.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit There is a word missing here

Copy link
Contributor

Choose a reason for hiding this comment

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

the test check is a good addition 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks - just reworded it. Let me know once you think everything looks good and we can get this merged and a new version published.

Copy link
Contributor

@AnthonyGlt AnthonyGlt left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍
We got to juggle with the notions of used and loaded but at the end, it makes the cache management more subtle.
Well done :D

@gkjohnson
Copy link
Contributor Author

Thanks - yeah nomenclature could probably use some work but I think the functionality is good.

@gkjohnson gkjohnson merged commit 7cd3bb5 into master Sep 30, 2024
2 checks passed
@gkjohnson gkjohnson deleted the mem-cache-fix branch September 30, 2024 08:42
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.

[REGRESSION] Implicit Tiling Repeating request
2 participants