-
-
Notifications
You must be signed in to change notification settings - Fork 928
Improve, optimize, and consolidate Tile LRU cache to new ES6 Map #6731
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6731 +/- ##
==========================================
- Coverage 92.21% 91.95% -0.26%
==========================================
Files 285 285
Lines 23742 23728 -14
Branches 5050 5045 -5
==========================================
- Hits 21894 21820 -74
- Misses 1848 1908 +60 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| } | ||
|
|
||
| // Updating same key with a new tile - remove old tile and add new one | ||
| this.remove(key); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this call onRemove? I'm honestly asking, I don't know the right answer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe so:
https://github.com/maplibre/maplibre-gl-js/blob/main/src/tile/tile_cache.ts#L82-L85
Eventually on remove would be called for the same id.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
onRemove propagates back to source.unloadTile which then goes back to the worker_source.removeTile. In here, loaded is used to store tiles by uid in the worker. uid is created in tile.js using uniqueId() which is an integer count++ on each function run, so every tile object has a new and unique uid. Even though the new tile may have the same tileID, it will absolutely have a unique uid, which means it still needs to be removed from the this.loaded cache in the worker. In other words, onRemove needs to be called otherwise the tiles would collect in the worker. Do some tracing and let me know if you agree...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I think I know what should be the right approach here in order to get confident in the current solution.
- We need to cover the current cache with tests so that we know exactly how it operates, which callbacks are called when and how it behaves in general (assuming this is not the tests that currently exists, if the tests cover the current functionality then we can skip this step).
- After we have the tests in place, we can simply replace the implementation under the hood - no changes to logic (besides maybe the timers), without even changing the public API (take vs getAndRemove, clear vs reset etc)
- After this is passing and we know that we haven't changed any logic, we can rename stuff.
I think this is the safest way to handle this.
If this is how you did it, then there's very low risk in introducing bugs, as the process above is fairly safe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've read the cache code start to end and a few questions rose:
- Why does it have an array? Most of the usage is referring to the first element, but not all.
- If the timeout were to be removed, how will this affect the code that listens to onRemove? What will happen if the tile's onRemove is not called when the tile expiries?
- The functionally as covered in the current tests (not those you added, but those that exist) is far from ideal, there's even a comment about how the cache should behave but it's not covered in a test, only in a comment, so not a great start...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep in mind that even though it says this._cache, I would think of it as this._viewportCache. It isn’t really a large tile cache it’s just a small cache for viewport rendering. It can't be that large because of the filter function which gets called when changing glyphs/symbols, so the cache size carries maintenance overhead.
While I am not completely certain:
- The array appears to either be an optimization using old objects to keep a separate order array, and/or it was also needed to maintain older tile references to access in onRemove via the timers.
- Tile expiration is just a timestamp, so when this time lapses the tile just sits in the cache. When it is removed via
take, the time is checked and if expired it is discarded and unloaded in_takeTileFromCache. - I think it's simply a different functionality. The newer cache is simpler because of Map and operates on the assumption that expired tiles can remain in the cache and evicted/unloaded when they are retrieved or the cache needs the space.
If you read the 3rd line here https://www.npmjs.com/package/lru-cache it also supports the idea that a cache should be either TTL or LRU, not both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to find a day or two without interruptions to look at the tile manager code thoroughly and see how to break it down, I hope I'll be able to find the time next week, sorry for dragging this.
The above statement is not relevant to changes you are doing, as I'll keep on reviewing them and provide feedback (as best I can, the code is too complicated without a good reason...).
Since ES6, Map has made simple in-memory caching far easier, providing fast average-case O(1) access. Our previous tile cache relied heavily on O(n) array operations. This PR replaces that implementation with the existing BoundedLRUCache and wires it into TileCache. I evaluated several npm caching libraries, but none fit well with the specifics of MapLibre’s tile loading model or offered a good balance of size and simplicity.
The most critical operation used in TileManager, getAndRemove, previously incurred three O(n) operations per call. It is now implemented as take, which performs the same behavior in O(1). Additionally, the old tile cache maintained many concurrent timers for tiles sitting idle in the cache. The new cache no longer tracks per-tile timers; instead, expired tiles are evicted/unloaded lazily on access, avoiding the overhead of a large number of active timers.
O(N) Comparison Table
Timers Comparison Table
CHANGELOG.mdunder the## mainsection.