This repository was archived by the owner on Feb 25, 2025. It is now read-only.
[Impeller] Started recycling vkRenderPass and vkFramebuffer #44861
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
Contributor
jonahwilliams
left a comment
jonahwilliams
left a comment
There was a problem hiding this comment.
You can't recycle the objects like this, you need to use the resource manager and wait for the fence waiter to be signal
Member
Author
It's getting tracked in the tracked resources so it won't be reused until the RingBufferItem is deleted (when the fence waiter signals). There is a bug in it right now which i'm fixing. |
gaaclarke
commented
Aug 18, 2023
| } | ||
|
|
||
| if (!encoder->Track(framebuffer) || !encoder->Track(render_pass)) { | ||
| if (!encoder->Track( |
Member
Author
There was a problem hiding this comment.
@jonahwilliams right here we are making sure it isn't placed back into the pool until the frame is done.
gaaclarke
force-pushed
the
pool-vkrenderpass-vkframebuffer
branch
from
3f29a1c to
7048839
Compare
Member
Author
|
This isn't quite right yet. I hadn't noticed that the vkrenderpass and vkframebuffer creation depends on the render target. So we will need something like a ring buffer per render target (while making sure we clean up old render targets). |
Member
Author
|
Closing this. After further investigation we've found this wasn't the culprit. |
Contributor
|
There are multiple culprits, here is a run where despite not being the slowest operation, creating the renderpass still took 3ms:
|
gaaclarke
force-pushed
the
pool-vkrenderpass-vkframebuffer
branch
from
7048839 to
723fe9c
Compare
8 tasks
gaaclarke
added a commit
that referenced
this pull request
Aug 25, 2023
…ayouts (#45088) This refactor makes recycling render passes easier since it starts to split out setting the layout of textures with creating the render pass description. I didn't fully implement the split since it would technically be slower. Until we get caching of render passes working it would require us to calculate the attachment descriptions twice, once to make the render pass and once to set the texture layout. teases out some of the work from: #44527 in the service of implementing: #44861 issue: flutter/flutter#133182 The final split will have this function for recycled render passes: ```c++ void SetTextureLayouts(const RenderTarget& render_target, const std::shared_ptr<CommandBufferVK>& command_buffer) { for (const auto& [bind_point, color] : render_target.GetColorAttachments()) { SetTextureLayout(color, CreateAttachmentDescription(color, &Attachment::texture), command_buffer, &Attachment::texture); if (color.resolve_texture) { SetTextureLayout( color, CreateAttachmentDescription(color, &Attachment::resolve_texture), command_buffer, &Attachment::resolve_texture); } } if (auto depth = render_target.GetDepthAttachment(); depth.has_value()) { SetTextureLayout( depth.value(), CreateAttachmentDescription(depth.value(), &Attachment::texture), command_buffer, &Attachment::texture); } if (auto stencil = render_target.GetStencilAttachment(); stencil.has_value()) { SetTextureLayout( stencil.value(), CreateAttachmentDescription(stencil.value(), &Attachment::texture), command_buffer, &Attachment::texture); } } ``` ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See [testing the engine] for instructions on writing and running engine tests. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I signed the [CLA]. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style [testing the engine]: https://github.com/flutter/flutter/wiki/Testing-the-engine [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat
gaaclarke
force-pushed
the
pool-vkrenderpass-vkframebuffer
branch
from
723fe9c to
e848d45
Compare
chinmaygarde
changed the title
gaaclarke
force-pushed
the
pool-vkrenderpass-vkframebuffer
branch
from
e848d45 to
7c54016
Compare
Member
Author
|
Current state: Disabling the cache has no validation errors, turning it on leads to validation errors surrounding the resolve texture. The is an image barrier to transition the layout, but I'm guessing that it is not valid. Potentially the source layout is wrong? A successful run looks like this too so I'm not sure what's different yet. |
Member
Author
|
It tried:
My theory is that the synchronization that we are doing for the other textures has to happen for the resolve texture. But what we are doing for the other textures won't work for the resolve texture since it is swapping between coloroptimal->present so we can't knowingly swap it from general back to whatever. We need to find a new way. Given that theory though, I'm at a loss to explain why it currently works. |
Member
Author
|
Here is the output now with the new logs: https://gist.github.com/gaaclarke/437c0753782601d0f78300977b7f2c46 It seems like the initialLayout for the resolve texture should be color optimal, or there should be a barrier to make it happen. |
gaaclarke
added a commit
to gaaclarke/engine
that referenced
this pull request
Aug 30, 2023
…ayouts (flutter#45088) This refactor makes recycling render passes easier since it starts to split out setting the layout of textures with creating the render pass description. I didn't fully implement the split since it would technically be slower. Until we get caching of render passes working it would require us to calculate the attachment descriptions twice, once to make the render pass and once to set the texture layout. teases out some of the work from: flutter#44527 in the service of implementing: flutter#44861 issue: flutter/flutter#133182 The final split will have this function for recycled render passes: ```c++ void SetTextureLayouts(const RenderTarget& render_target, const std::shared_ptr<CommandBufferVK>& command_buffer) { for (const auto& [bind_point, color] : render_target.GetColorAttachments()) { SetTextureLayout(color, CreateAttachmentDescription(color, &Attachment::texture), command_buffer, &Attachment::texture); if (color.resolve_texture) { SetTextureLayout( color, CreateAttachmentDescription(color, &Attachment::resolve_texture), command_buffer, &Attachment::resolve_texture); } } if (auto depth = render_target.GetDepthAttachment(); depth.has_value()) { SetTextureLayout( depth.value(), CreateAttachmentDescription(depth.value(), &Attachment::texture), command_buffer, &Attachment::texture); } if (auto stencil = render_target.GetStencilAttachment(); stencil.has_value()) { SetTextureLayout( stencil.value(), CreateAttachmentDescription(stencil.value(), &Attachment::texture), command_buffer, &Attachment::texture); } } ``` ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See [testing the engine] for instructions on writing and running engine tests. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I signed the [CLA]. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style [testing the engine]: https://github.com/flutter/flutter/wiki/Testing-the-engine [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat
Contributor
|
Okay I feel dumb, the problem seems to be that the framebuffer objects have references to the actual attachments. So if you recycle them and the attachments aren't the same its just straight up invalid - meaning we always have to recreate them. IN theory the RenderPass is only referencing the attachments in the framebuffer by index, so that may work. Still looking so basically you can only reuse the framebuffer if you have a static layout with a fixed number of objects per frame |
Member
Author
|
My thought process was that render passes should just have a layout which considering the rendertargets are equal, should be the same. Based on the trace statements we can see that the rendertargets are the same so I didn't implement that part of the caching yet. I had considered framebufferrs will be harder to recycle since they may have links to swapchain images. But since the errors I was seeing were related to resolve texture image layouts, which is the province of the renderpass and pipeline barriers I didn't spend much time looking into the framebuffer. However, allocating the framebuffer is the one that I measured as being slow so if we can't easily recycle that, the value of this PR decreases. |
Member
Author
|
Closing this PR to clear out the triage process. I requested that this policy be reviewed since my hope would be that drafts are not reviewed. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
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.
Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.
List which issues are fixed by this PR. You must list at least one issue.
If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.