[Impeller] avoid hashing and std::vector growth when binding descriptor sets. #45070
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). 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. |
|
Can/should we add a hard limit to the binding count somewhere? |
|
|
||
| std::unordered_map<uint32_t, vk::DescriptorBufferInfo> buffers; | ||
| std::unordered_map<uint32_t, vk::DescriptorImageInfo> images; | ||
| vk::DescriptorImageInfo images[32]; |
There was a problem hiding this comment.
Would it be too expensive to use a vector and reserve space based on the size of vertex_bindings/fragment_bindings?
There was a problem hiding this comment.
No I think that is probably a better idea. We should be able to assert on the size to make sure the ptrs are valid
| write_set.descriptorType = vk::DescriptorType::eCombinedImageSampler; | ||
| write_set.pImageInfo = &(images[slot.binding] = image_info); | ||
| FML_DCHECK(image_count < 32u); | ||
| write_set.pImageInfo = &(images[image_count] = image_info); |
There was a problem hiding this comment.
The old implementation would only store one entry if multiple images used the same slot.binding index. Is that behavior desirable?
There was a problem hiding this comment.
The bindings shouldn't be duplicated, but even if they were the image/buffer map we are processing should have already dedicated (I think)
chinmaygarde
changed the title
jonahwilliams
added
the
autosubmit
…133342) flutter/engine@3dcd217...1471967 2023-08-25 [email protected] Update gradle to 7.5.1 (flutter/engine#45113) 2023-08-25 [email protected] Ignore unguarded-availability for unit test (flutter/engine#44852) 2023-08-25 [email protected] [Impeller] avoid hashing and std::vector growth when binding descriptor sets. (flutter/engine#45070) 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],[email protected],[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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…or sets. (flutter#45070) We call allocateAndBindDescriptorSets once for every cmd rendered, so this time can bubble up quite a bit. From the traces I've gathered map emplacement (emplace_unique_key_args) and std::vector growth (__push_back_slow_path) contribute the most to this trace. I think we have physical limits on the number of descriptors we can bind, though I'm not sure if we enforce that anywhere?
2 participants
We call allocateAndBindDescriptorSets once for every cmd rendered, so this time can bubble up quite a bit. From the traces I've gathered map emplacement (emplace_unique_key_args) and std::vector growth (__push_back_slow_path) contribute the most to this trace.
I think we have physical limits on the number of descriptors we can bind, though I'm not sure if we enforce that anywhere?