LRU-style Texture Memory Management #301
Conversation
frank-weindel
requested review from
wouterlucas,
jfboeve,
michielvandergeest and
erikhaandrikman
frank-weindel
added
the
breaking change!
src/core/CoreTextureManager.ts
Outdated
| const { inverseKeyCache, keyCache } = this; | ||
| const cacheKey = inverseKeyCache.get(texture); | ||
| if (cacheKey) { | ||
| inverseKeyCache.delete(texture); |
There was a problem hiding this comment.
You probably don't need to clear WeakMap entries
| if (!isIdle) { | ||
| stage.emit('idle'); | ||
| if (stage.txMemManager.checkCleanup()) { |
There was a problem hiding this comment.
I'm a bit concerned with this approach:
- with L2, texture cleanup would usually happen only reaching the critical level, so scrolling down then up a screen full of rails will only have visible textures reloading after we hit the ceiling (possibly never),
- with L3 it will tend to cleanup both when reaching critical level AND every X seconds (&idle), which means it will effectively feel like the device has only a fraction of the GPU texture capacity. I appreciate the delay is configurable, but 5s default seem aggressive.
There was a problem hiding this comment.
The new L3 approach here is also only freeing down to the Target Threshold (in LRU order) instead of all non-renderable textures. It actually shouldn't run on idle if the memory use is lower than the Target Threshold. I forgot to add a check for that.
To emulate L2 behavior, I think you could set the Target Threshold to 0 and Cleanup Interval to Infinity. But the sorting by LRU would be pointless in that case.
How does this approach compare to what you were hoping for in #233?
There was a problem hiding this comment.
It's certainly going to enable what we were "hacking" with L2 - that's great to see!
Just saying that the default 5s, while it can be changed, is probably too frequent: for most apps it will divide the effective memory pressure and defeat the point of caching.
There was a problem hiding this comment.
I've increased the default to 30 seconds. I really don't have a sense of what it should be. But we can adjust in the future as needed.
- Adds `monitor` boolean URL param to examples that shows a memory monitor. Resolves #233 BREAKING CHANGE
- Raise default cleanupInterval to 30 seconds - Add check to not run Idle Cleanup if Target Threshold is not exceeded - Remove unnecessary delete from WeakMap
frank-weindel
force-pushed
the
feat/lru-mem-management
branch
from
c8f135a
to
2b221cb
Compare
MemMonitor.mov
TextureMemoryManagerSettingsfor info on the new configuration settings.monitorboolean URL param to examples that shows a memory monitor.Resolves #233
BREAKING CHANGE