Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upReact to memory pressure. #1552
Conversation
|
This built on top of #1532 so it has some extra commits until that PR is merged. |
|
|
nical
force-pushed the
nical:mem-pressure
branch
2 times, most recently
from
bc0b9df
to
1ff0b58
|
Rebased and tested. You can use the 'm' key in the sample and in wrench to trigger memory pressure events. |
|
What happened to the idea of having the expiration time being a non-linear function of the available memory? It seems like it would ultimately replace the proposed memory pressure mechanics. |
| @@ -89,6 +90,14 @@ impl GlyphCache { | |||
| } | |||
| } | |||
|
|
|||
| pub fn clear(&mut self, texture_cache: &mut TextureCache) { | |||
| let caches = mem::replace(&mut self.glyph_key_caches, FastHashMap::default()); | |||
This comment has been minimized.
This comment has been minimized.
kvark
Aug 8, 2017
Member
this could be rewritten cleaner without mem::replace, which I consider highly overused in our codebase, e.g.:
for (_, mut glyph_key_cache) in &mut self.glyph_key_caches {
glyph_key_cache.clear(texture_cache);
}
self.glyph_key_caches.clear();Bonus points - space is kept reserved.
This comment has been minimized.
This comment has been minimized.
nical
Aug 8, 2017
Author
Collaborator
Bonus points - space is kept reserved.
Well, this is the one situation where keeping the space reserved is undesirable. But I can just do the loop as you suggest and assign an empty hash map to it instead of clearing if you prefer.
This comment has been minimized.
This comment has been minimized.
| @@ -268,6 +268,7 @@ impl RendererFrame { | |||
| pub enum ResultMsg { | |||
| RefreshShader(PathBuf), | |||
| NewFrame(DocumentId, RendererFrame, TextureUpdateList, BackendProfileCounters), | |||
| UpdateResources(TextureUpdateList, bool /* cancel rendering */), | |||
This comment has been minimized.
This comment has been minimized.
I think that we should do both. There are other places where we could free up memory beyond the texture cache (I started with it because it was the obvious one, but we should extend this to other places). Because of memory fragmentation, it can be hard to tell from the amount of available memory whether we are really in trouble and allocations can start failing (if we are lucky causing memory pressure events) before we detect that we are really in trouble if we only look at the available memory. I see the idea of adjusting expiration frames in function of the available memory more as a way to keep the memory usage "reasonable" (whatever that means) in general, while memory pressures are a last resort stability thing. |
|
@bors-servo r+ |
|
|
React to memory pressure. Memory pressure events are sent when the amount of available memory is so low that we risk crashing very soon if we don't do anything about it. When this happens we don't really care about frame budgets and smoothness anymore, we can only do a best effort at freeing memory and hope to not crash. This PR adds memory pressure notifications to the API and reacts to it by completely clearing the texture cache and clearing some of the allocations that we try to recycle for performance purposes.
|
|
nical commentedAug 4, 2017
Memory pressure events are sent when the amount of available memory is so low that we risk crashing very soon if we don't do anything about it. When this happens we don't really care about frame budgets and smoothness anymore, we can only do a best effort at freeing memory and hope to not crash.
This PR adds memory pressure notifications to the API and reacts to it by completely clearing the texture cache and clearing some of the allocations that we try to recycle for performance purposes.