[Impeller] dont print format strings for blend filter and snapshots. #57105
Conversation
jonahwilliams
changed the title
|
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. |
| @@ -174,8 +190,7 @@ static std::optional<Entity> AdvancedBlend( | |||
| std::invoke(pipeline_proc, renderer, options); | |||
|
|
|||
| #ifdef IMPELLER_DEBUG | |||
| pass.SetCommandLabel( | |||
| SPrintF("Advanced Blend Filter (%s)", BlendModeToString(blend_mode))); | |||
| pass.SetCommandLabel(BlendModeToFilterString(blend_mode)); | |||
| std::nullopt, // sampler_descriptor | ||
| msaa_enabled_, // msaa_enabled | ||
| /*mip_count=*/mip_count, | ||
| SPrintF("Contents to %s Filter Snapshot", label.c_str())); // label |
There was a problem hiding this comment.
Instead of removing this argument, can we just send the argument directly here. That way we'd remove the sprintf but keep valuable debugging information?
There was a problem hiding this comment.
It always gets sprintf'd with the existing texture label. I just don't think these are useful - you can already see the pipeline in the debugger.
and if we do find out we need them its easy to add back.
There was a problem hiding this comment.
These are printed out in validation logs too, you won't always have the opportunity to inspect the pipeline. Where ever it is getting sprintf's? We should be able to just take a string verbatim instead of trying to modify it. That's more useful than having no label. If we are trying to debug a customer's issue it won't be easy to add them back in.
There was a problem hiding this comment.
@gaaclarke do you use this feature? I don't . If it turns out to be useful, we can add it back.
There was a problem hiding this comment.
Yes, I've benefited from having debug labels when responding to a warning printed out from validation layers. We've had vulkan in a good state for a while I think we've forgotten how useful they can be. Respectfully, I think you are throwing out the baby with the bath water. The expensive part is the sprintf/malloc/free calls, not the actual labelling. I'd rather we throw those out first and remeasure.
There was a problem hiding this comment.
This is not a baby or bathwater. its a debug label. If we need it, we can add it back.
There was a problem hiding this comment.
Can we keep the string literals in the source code and conditionally use them? Then the compiler will just rip them out if they aren't used. They aren't going to be easy to bring back. Bringing them back would be recreating something we already had, it will be easier for us to just maintain them. Any patch will bit rot pretty fast.
There was a problem hiding this comment.
You PR that landed today basically did that, right? It makes it so naming textures is a noop, so can we just keep the literals in the code to make them easy to turn on?
| std::nullopt, // sampler_descriptor | ||
| true, // msaa_enabled | ||
| /*mip_count=*/mip_count, | ||
| SPrintF("Filter to %s Filter Snapshot", label.c_str())); // label |
We can use a macro to distinguish between all of the blend modes. We don't need to distinguish between porter duff/ advanced /pipeline as the pipeline is already labeled with the shader used.
For all snapshots the additional label on the texture isn't useful since we can just look at the command.