Skip to content

Conversation

@wayofthefuture
Copy link
Collaborator

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

Operation Complexity (v1 → v2) Potential Speedup
Get O(1) → O(1) ~1×
Set O(1) → O(1) ~1×
getAndRemove / Take O(N)O(1) Up to N× faster
Remove (by key) O(N)O(1) Up to N× faster
Filter O(N²)O(N) Up to N× faster
Resize (setMaxSize) O(N²)O(N) Up to N× faster
Reset / Clear O(N) → O(N) ~1×

Timers Comparison Table

Aspect v1 (Timer-based cache) v2 (LRU cache, no timers)
Timers per tile 1 per entry (hundreds–thousands) 0
Event-loop overhead High (timer scheduling + callbacks) None
GC pressure High (many closures + timer objects) Low
Callback churn Thousands of callbacks None
Memory per tile Large (timeout + closure) Small (Map entry only)
Latency/jank risk High Low
  • Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
  • Briefly describe the changes in this PR.
  • Write tests for all new functionality.
  • Add an entry to CHANGELOG.md under the ## main section.

@codecov
Copy link

codecov bot commented Nov 17, 2025

Codecov Report

❌ Patch coverage is 96.96970% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.95%. Comparing base (767a245) to head (59be754).
⚠️ Report is 13 commits behind head on main.

Files with missing lines Patch % Lines
src/tile/tile_manager.ts 93.54% 2 Missing ⚠️
src/style/style.ts 0.00% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

}

// Updating same key with a new tile - remove old tile and add new one
this.remove(key);
Copy link
Collaborator

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.

Copy link
Collaborator Author

@wayofthefuture wayofthefuture Nov 17, 2025

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.

Copy link
Collaborator Author

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...

Copy link
Collaborator

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.

  1. 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).
  2. 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)
  3. 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.

Copy link
Collaborator

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:

  1. Why does it have an array? Most of the usage is referring to the first element, but not all.
  2. 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?
  3. 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...

Copy link
Collaborator Author

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:

  1. 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.
  2. 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.
  3. 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.

Copy link
Collaborator

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...).

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.

2 participants