[Impeller] dont use dynamic shader metadata path for precompiled shaders. #56827
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
|
|
||
| for (const auto& uniform : runtime_stage_->GetUniforms()) { | ||
| std::shared_ptr<ShaderMetadata> metadata = MakeShaderMetadata(uniform); | ||
| ShaderMetadata metadata = MakeShaderMetadata(uniform); |
There was a problem hiding this comment.
this doesn't need to be a shared_ptr since its going to be copied into the bindings object anyway.
gaaclarke
left a comment
gaaclarke
left a comment
There was a problem hiding this comment.
Looking good, just needs docstrings to show the difference between the overloaded calls, remove some duplication. I have a few suggestions for performance but they aren't important since they are for runtime effects.
| ShaderMetadata metadata; | ||
| metadata.name = uniform.name; | ||
| metadata.members.emplace_back(ShaderStructMemberMetadata{ |
There was a problem hiding this comment.
nit: it would be nice to verify that we are getting the return value optimization here, an rvalue in the return statement would make this more likely.
There was a problem hiding this comment.
Made this unique_ptr anyway
| } | ||
|
|
||
| static std::shared_ptr<ShaderMetadata> MakeShaderMetadata( | ||
| static ShaderMetadata MakeShaderMetadata( |
There was a problem hiding this comment.
nit: I think we should move this to std::unique_ptr<ShaderMetadata>. That way we can pass ownership to the Resource. We could use std::move with ShaderMetadata but that isn't enforced like unique_ptr is. It's only a nit since it only effects runtime effect's performance.
impeller/renderer/render_pass.h
Outdated
| virtual bool BindResource(ShaderStage stage, | ||
| DescriptorType type, | ||
| const ShaderUniformSlot& slot, | ||
| const ShaderMetadata& metadata, | ||
| BufferView view); |
There was a problem hiding this comment.
This is so close to the other BindResource, it's probably worth documenting.
impeller/renderer/command.h
Outdated
| bool BindResource(ShaderStage stage, | ||
| DescriptorType type, | ||
| const SampledImageSlot& slot, | ||
| const ShaderMetadata& metadata, | ||
| std::shared_ptr<const Texture> texture, | ||
| const std::unique_ptr<const Sampler>& sampler); | ||
|
|
There was a problem hiding this comment.
Same thing here, worth documenting since it's not obvious the difference with the other one.
impeller/renderer/command.cc
Outdated
| if (!sampler) { | ||
| return false; | ||
| } | ||
| if (!texture || !texture->IsValid()) { | ||
| return false; | ||
| } | ||
|
|
||
| switch (stage) { | ||
| case ShaderStage::kVertex: | ||
| vertex_bindings.sampled_images.emplace_back(TextureAndSampler{ | ||
| .slot = slot, | ||
| .texture = TextureResource::MakeDynamic(metadata, std::move(texture)), | ||
| .sampler = &sampler, | ||
| }); | ||
| return true; | ||
| case ShaderStage::kFragment: | ||
| fragment_bindings.sampled_images.emplace_back(TextureAndSampler{ | ||
| .slot = slot, | ||
| .texture = TextureResource::MakeDynamic(metadata, std::move(texture)), | ||
| .sampler = &sampler, | ||
| }); | ||
| return true; | ||
| case ShaderStage::kCompute: | ||
| VALIDATION_LOG << "Use ComputeCommands for compute shader stages."; | ||
| case ShaderStage::kUnknown: | ||
| return false; | ||
| } | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
Please remove duplication with the above method.
|
Changed the binding methods to BindDynamic* so its not just an overload anymore |
jonahwilliams
added
the
autosubmit
…159563) flutter/engine@ed4e13c...ba112ad 2024-11-27 [email protected] [Impeller] dont use dynamic shader metadata path for precompiled shaders. (flutter/engine#56827) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…ers. (flutter/engine#56827) Should fix flutter#159520 ~~but I still need to check locally.~~ Seems to do the trick. All cmd bindings were copying the shader metadata, which meant allocating/de-allocating a lot of strings per draw.
2 participants
Should fix flutter/flutter#159520
but I still need to check locally.Seems to do the trick.All cmd bindings were copying the shader metadata, which meant allocating/de-allocating a lot of strings per draw.