vello_hybrid: Use RAII pattern for handling WebGL resources#1674
Open
LaurenzV wants to merge 2 commits into
Open
vello_hybrid: Use RAII pattern for handling WebGL resources#1674LaurenzV wants to merge 2 commits into
LaurenzV wants to merge 2 commits into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a new PR, which does basically the same as #1574 but is based on the newest main, which has had quite a few changes. To explain the motivation again:
Motivation
Right now, the WebGL backend of Vello Hybrid creates various resources, such as buffers, textures, etc., but we never actually destroy them once they go out-of-scope. This means that we are essentially relying on the garbage collector to properly clean those up. This might be fine if we are only creating a single Vello Hybrid instance, but 1) it's bad practice and 2) might cause problems in case you want to have the ability to create multiple Vello Hybrid (for example to recreate it after a WebGL context loss).
Implementation
Basically, what this PR proposes is to create a wrapper type for all resources, each resource implementing a trait that defines how to create the resource and how to drop it. We then make use of the
Dropimplementation ofResourceto manually delete the resource again. This way, as soon as the variable goes out-of-scope (since it's not used anymore), the bound resource will be deleted as well. This way, we don't have to worry about manually deleting them anymore.I should note that there is a small footgun we need to be careful about: It is still possible to write code that for example assumes that some resource has been bound previously and then acts on that. It could happen that we create a new resource, bind it in WebGL, then the variable goes out-of-scope and some other function later on still acts on that binding, assuming that it's still alive. I'm not sure what would happen in such a case, but it's certainly not good. As far as I can tell, we currently don't have any such code (I rewrote the
upload_data_to_rgba32_texturemethod to let it take care of binding the resource it needs explicitly), but it's still something we should keep in mind when extending the backend in the future. I'm not sure if there is any way of preventing such code at compile-time via a different design. Nevertheless, I think this is a good start.Testing
I have tried the WebGL example scenes on my machine and also ran vello_bench2 on a couple of devices, and didn't observe any problems.