cleanup texture cache #1064
cleanup texture cache #1064
Conversation
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## master #1064 +/- ##
==========================================
+ Coverage 24.00% 24.11% +0.10%
==========================================
Files 140 140
Lines 21562 21481 -81
==========================================
+ Hits 5177 5180 +3
+ Misses 16385 16301 -84
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
The first commit is certainly fine this way (apart from docs). I don't love the second, but I am not sure, if calling cleanup_texture_cache all the time is a better solution. It probably not negligible in terms of performance. Can we maybe do a more explicit "no rendering happened"-state than using damage.is_none as a proxy for that information.
src/backend/drm/compositor/mod.rs
Outdated
| // if we receive no damage we can assume no rendering took place | ||
| // and we should trigger a cleanup of the renderer texture cache | ||
| // to prevent holding textures longer then necessary | ||
| renderer.cleanup_texture_cache(); |
There was a problem hiding this comment.
So that means, we kinda code the DrmCompositor around the assumption, that rendering will implicitly cleanup the texture cache? I kinda don't like that tbh, but I don't have a much better solution at hand.
There was a problem hiding this comment.
This needs to be at least very well documented on the cleanup_texture_cache method.
There was a problem hiding this comment.
Yeah, I don't like that either. But we are currently doing implicit cleanup which is probably OK. Removing that and relying on explicit cleanup with a separate function is worse imo. I am not sure there is an easy way to make the explicit cleanup less costly by only invoking it when really needed. But without it we potentially keep a lot of memory alive unnecessarily. Doing it the way I implemented will at least make sure we do not cleanup twice. I can extend the result of the output damage tracker to explicit tell us if rendering took place. But with the current api I don't see a way on how to encode the cleanup requirements (including the implicit cleanup) other then documenting it in the renderer trait docs. Would that be enough? Clarifying the implicit cleanup in the docs and adding a renderer bool to the result?
There was a problem hiding this comment.
Clarifying the implicit cleanup in the docs and adding a renderer bool to the result?
Yeah I would be fine with it as an immediate measure. Maybe when the vulkan renderer finally materializes we can take a step back and evaluate that approach again.
There was a problem hiding this comment.
Okay, so while I am all for merging this, can we at least add a note of implicit cleanup happening on Frame::drop to the Frame-trait and Renderer::cleanup_texture_cache docs?
There was a problem hiding this comment.
Okay, I added some comment to the trait and the cleanup function.
cmeissl
force-pushed
the
fix/cleanup_texture_cache
branch
from
8ff2320 to
83731bd
Compare
cmeissl
force-pushed
the
fix/cleanup_texture_cache
branch
5 times, most recently
from
22f37c8 to
22735c7
Compare
cmeissl
force-pushed
the
fix/cleanup_texture_cache
branch
from
22735c7 to
60483f4
Compare
cmeissl
force-pushed
the
fix/cleanup_texture_cache
branch
from
60483f4 to
feaef3a
Compare
| @@ -1224,6 +1240,10 @@ where | |||
| ) | |||
| .map_err(Error::Target)?; | |||
| let sync = frame.finish().map_err(Error::Target)?; | |||
| render | |||
There was a problem hiding this comment.
Just to clarify, why does this call cleanup_texture_cache() explicitly, even though the new docs say that Frame::finish() does it automatically?
There was a problem hiding this comment.
The frame here is from the target, so the call to cleanup_texture_cache should clean-up the other renderer in this case.
No description provided.