Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

[core] Evict unused font stacks from GlyphManager #12414

Merged
merged 1 commit into from
Aug 17, 2018
Merged

Conversation

jfirebaugh
Copy link
Contributor

Fixes #12388.

@ChrisLoer
Copy link
Contributor

This looks like a good approach to me, but did you also consider something like an LRU eviction strategy? I haven't thought it through too much, but I guess pros would be:

  • GlyphManager wouldn't be tied to being used with a particular style (or if you wanted to use it with multiple styles, you wouldn't have to keep track of the union of their fontstacks). Although right now the GlyphManager is tied to the style by the renderer, I can imagine us wanting to share a single GlyphManager w/ separate renderers (e.g. for MapSnapshotter)
  • The Layer '%s' has an invalid value for text-font and will not work offline. error wouldn't have to turn into a hard error
  • You might potentially use it to constrain memory use even in the single style case (imagine a multi-font map with lots of Chinese characters -- you could end up with a lot of data in memory, although without a disk cache, it would suck to discard the glyphs only to fetch them back from the network...)

Cons:

  • Maybe more complicated book-keeping?
  • Maybe doesn't match as well what you actually want to hold onto. e.g. if you're under the LRU cap but you've stopped using a fontstack, holding onto it is still a waste, and if you're over the LRU cap even with just one style, then maybe you'd rather use memory than discard glyphs.

@jfirebaugh
Copy link
Contributor Author

LRU doesn't really feel like the right fit to me. We don't want glyphs to get evicted when they may still be required, and we don't want them to stick around when they're no longer needed.

Plus it would lead to a more complicated implementation, and we'd have to spend some time figuring out what a reasonable cache limit is.

@kkaefer kkaefer added the Core The cross-platform C++ core, aka mbgl label Jul 31, 2018
@jfirebaugh
Copy link
Contributor Author

This has been incorporated in the node v4 beta with success; let's merge it to master.

Copy link
Contributor

@ChrisLoer ChrisLoer left a comment

Choose a reason for hiding this comment

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

👍

@jfirebaugh jfirebaugh merged commit 8ca419f into master Aug 17, 2018
@jfirebaugh jfirebaugh deleted the fix-12388 branch August 17, 2018 16:38
@jfirebaugh jfirebaugh restored the fix-12388 branch August 17, 2018 20:42
@jfirebaugh jfirebaugh deleted the fix-12388 branch August 17, 2018 20:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Core The cross-platform C++ core, aka mbgl
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants