[impeller] combine sampler and texture maps. #44990
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. |
| @@ -61,12 +61,17 @@ using BufferResource = Resource<BufferView>; | |||
| using TextureResource = Resource<std::shared_ptr<const Texture>>; | |||
| using SamplerResource = Resource<std::shared_ptr<const Sampler>>; | |||
|
|
|||
| /// @brief combines the texture, sampler and sampler slot information. | |||
| struct TextureAndSampler { | |||
There was a problem hiding this comment.
This doesn't make the memory usage smaller. The Command size is smaller, but the map entries will be bigger. The way to reduce memory would be to use std::variant so that each entry is just the max of the different sizes plus the type signifier.
I tried this in the past (main...gaaclarke:engine:command-thinning) and found that the overhead of switching on the std::variant ate up the savings from smaller memory. Plus, std::variants are super cumbersome to work with.
There was a problem hiding this comment.
we don't need a std::variant, and its not just about the memory usage. The cost of computing hashes (std::find in the profile) is eating up CPU time.
There was a problem hiding this comment.
Why not just go to vectors in this patch?
|
It will help, because we're not hashing a bunch of times for the same data. Its not about the size of the map |
jonahwilliams
added
the
autosubmit
| bindings.samplers[slot.sampler_index] = {&metadata, sampler}; | ||
| return true; | ||
| } | ||
| bindings.sampled_images[slot.sampler_index] = TextureAndSampler{ |
There was a problem hiding this comment.
I got you, this specifically is batching the hashes at creation time for the command. When we execute we are just looping over them anyways so it doesn't matter. SGTM.
Follow up from #44990 We're spending a ton of time deallocating mostly empty maps, by reducing the number of maps we reduce the amount of memory used, and reduce map lookups.
…133211) flutter/engine@58dc868...27d75f6 2023-08-23 [email protected] Roll Skia from 76898dad9fda to a631fefdba37 (2 revisions) (flutter/engine#45027) 2023-08-23 [email protected] Revert "FontVariation.lerp, custom FontVariation constructors, and more documentation" (flutter/engine#45023) 2023-08-23 [email protected] [Impeller] Dat rvalue reference (fix engine head) (flutter/engine#45024) 2023-08-23 [email protected] Revert "Enable clang-tidy for pre-push (opt-out), exclude `performance-unnecessary-value-param`" (flutter/engine#45020) 2023-08-23 [email protected] Roll Skia from 4e42b51cfe27 to 76898dad9fda (1 revision) (flutter/engine#45019) 2023-08-23 [email protected] [Impeller] Add STB text backend. (flutter/engine#44887) 2023-08-23 [email protected] Followup to flutter/engine#44982 (flutter/engine#45018) 2023-08-23 [email protected] Roll Skia from 5428f147e632 to 4e42b51cfe27 (4 revisions) (flutter/engine#45016) 2023-08-23 [email protected] Eliminate android test log spam (flutter/engine#44982) 2023-08-23 [email protected] [web] Remove some unused functions (flutter/engine#44505) 2023-08-23 [email protected] Use decal TileMode in blur image_filter_test.dart (flutter/engine#45004) 2023-08-23 [email protected] FontVariation.lerp, custom FontVariation constructors, and more documentation (flutter/engine#44996) 2023-08-23 [email protected] [impeller] combine sampler and texture maps. (flutter/engine#44990) 2023-08-23 [email protected] [Impeller] Flutter GPU: Add HostBuffer. (flutter/engine#44696) 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
Simplify command encoding and reduce binding size by placing all texture/sampler data in a single map instead of 3. We don't currently (nor do we plan to) support separate textures and samplers. The vulkan backend is particularly bad, because there are 3 map lookups to pull all of the texture and sampler data out of the bindings. Also see [go/impeller-vulkan-cmd-recording-performance](http://goto.google.com/impeller-vulkan-cmd-recording-performance)
Follow up from flutter#44990 We're spending a ton of time deallocating mostly empty maps, by reducing the number of maps we reduce the amount of memory used, and reduce map lookups.
Simplify command encoding and reduce binding size by placing all texture/sampler data in a single map instead of 3. We don't currently (nor do we plan to) support separate textures and samplers.
The vulkan backend is particularly bad, because there are 3 map lookups to pull all of the texture and sampler data out of the bindings.
Also see go/impeller-vulkan-cmd-recording-performance