-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[core] defer buffer/texture destruction while used by a command buffer #8706
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
CTS still hangs on Mac, I need to investigate this |
|
The known cause of CTS hangs on Mac is #3084, although that doesn't totally make sense here, because the test list hasn't changed. |
|
Is this approach the right path forward ? Let me add my measurements regarding the auto-retain or not flag and memory usage later today |
f44dcdb to
fe9eb2d
Compare
|
@andyleiserson rebased to get your CTS filter changes, it's now hanging with |
|
I'm not sure about the approach. There is already a mechanism to defer destruction for buffers/textures that are in use -- see the code at the end of It may be useful to look at #8129 and the "full set of changes" referenced in the description, in particular 42e4a04. This was a (never merged) attempt to address the resource lifetime problem a different way closer to what you are doing, where the resources would be kept alive via I don't think it should be necessary to have both the I would still be interested to see a test case. We have made changes over the past few months (in particular, encoding on finish) that should have made it harder to destroy resources and then still end up trying to use a command buffer that references them. The exception I know of is #7816, I believe (other than setting the retain references flag) that issue is still outstanding and I'm not aware of a strategy for fixing it besides keeping all resources alive whenever they are referenced in a command buffer. But I think that might be a Metal bug, and I don't know if we want to remove the ability to eagerly destroy resources entirely if we can get it fixed in Metal. |
|
Thank you for the detailed explanation. I clearly didn't had enough context when approaching this but what I'm sure about is that #7816 doesn't happen anymore with this proposed approach (and the previous closed one), even with unretained references (without #7842).
Unfortunately all I have is memory measurements from our iOS app and #7842 is definitely the culprit: memory indefinitely accumulate when this specific commit is cherry-picked. But that's on
In the end I'm not even looking for this exact proposed approach/behavior, I was just trying to get #7816 fixed without the need for #7842. Hopping that it can fix the following as well: Let me re-run my measurements against
This appear to be true, but the validation error above make me think that something might still be wrong in |
|
Closing as I'm unable to reproduce on trunk, sounds like it has been fixed already. I should have started from there directly - sorry for the time loss. |
|
No worries. I do think that turning the retained references flag back off is desirable -- I just haven't had time to dig into it. Re: the |
Description
Allow setting
retain_command_buffer_referencesback to false by deferring buffer and texture destroy if used by a command buffer. Replace #8694Testing
CTS and tests
Checklist
cargo fmt.Runtaplo format.cargo clippy --tests. If applicable, add:--target wasm32-unknown-unknowncargo xtask testto run tests.If this contains user-facing changes, add aCHANGELOG.mdentry.