[Impeller] remove Vulkan pipeline cache mutex. #43085
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. |
jonahwilliams
requested review from
chinmaygarde and
dnfield
and removed request for
dnfield
|
If we end up creating a bunch of variants on the fly this would also explain why it sometimes takes longer. |
| // an extension. Check it and set it. If not, the implementation is doing | ||
| // unnecessary synchronization. | ||
| // cache_info.flags = | ||
| // vk::PipelineCacheCreateFlagBits::eExternallySynchronized; |
There was a problem hiding this comment.
Based on what I'm reading in https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/VkPipelineCacheCreateFlagBits.html it sounds like the idea would be to use this extension so we could use the mutex ourselves.
But since we don't have the extension it ends up being both us and the implementation blocking.
So we might as well just let the implementation do it for us.
dnfield
left a comment
dnfield
left a comment
There was a problem hiding this comment.
LGTM.
Another quote
VUID-vkCreateGraphicsPipelines-pipelineCache-02876
If pipelineCache was created with VK_PIPELINE_CACHE_CREATE_EXTERNALLY_SYNCHRONIZED_BIT, host access to pipelineCache must be externally synchronized
|
Actually per https://registry.khronos.org/vulkan/specs/1.3-extensions/html/vkspec.html#pipelines-cache-destroy it looks like we still have to synchronize the destruction of the cache. |
|
...but we haven't been doing that anyway. |
|
I think we can get away with that since most/all of our Vk objects should be destructed on the raster thread, but I don't know. |
jonahwilliams
added
the
autosubmit
|
The pipeline cache can get destroyed either on the raster thread or on the worker thread that persists the cache to disk. |
…129378) flutter/engine@703c9a1...ed477de 2023-06-22 [email protected] Roll ANGLE from bbcf54bcb738 to 4ed2d403a329 (7 revisions) (flutter/engine#43105) 2023-06-22 [email protected] Workaround a release blocker after libc++ change (flutter/engine#43091) 2023-06-22 [email protected] Roll Skia from 8168c802c391 to 09b36b8ce0db (1 revision) (flutter/engine#43102) 2023-06-22 [email protected] [Impeller] Reland: Correctly compute UVs in texture fill (flutter/engine#43093) 2023-06-22 [email protected] [Impeller] Add validation forbidding SamplerAddressMode::kDecal on the OpenGLES backend (flutter/engine#43094) 2023-06-22 [email protected] Use minor version, ignore patches for CodeQL (flutter/engine#43088) 2023-06-22 [email protected] Print a warning when a message channel is used on the wrong thread. (flutter/engine#42928) 2023-06-22 [email protected] Roll Skia from 3f3e1da4b7eb to 8168c802c391 (4 revisions) (flutter/engine#43096) 2023-06-22 [email protected] [Impeller] default sample count back to 1 (but configure to 4 in defaults). (flutter/engine#43089) 2023-06-22 [email protected] [web] Don't get break type from v8BreakIterator (flutter/engine#43053) 2023-06-22 [email protected] Roll dart to 3.1.0-239.0.dev (flutter/engine#43083) 2023-06-22 [email protected] Revert "[Impeller] dont use concurrent runner to decode images on Android." (flutter/engine#43061) 2023-06-22 [email protected] [Impeller] Add fence waiter trace event. (flutter/engine#43092) 2023-06-22 [email protected] [Impeller] remove Vulkan pipeline cache mutex. (flutter/engine#43085) 2023-06-22 [email protected] Revert "[Impeller] Correctly compute UVs in texture fill" (flutter/engine#43087) 2023-06-22 [email protected] Roll Fuchsia Linux SDK from 7EZeNE4aGd29VfDly... to tcVndpnH_jzGm5LsJ... (flutter/engine#43081) 2023-06-22 [email protected] Roll Skia from 117f57a53215 to 3f3e1da4b7eb (4 revisions) (flutter/engine#43080) 2023-06-22 [email protected] Roll ANGLE from 7658525166a4 to bbcf54bcb738 (1 revision) (flutter/engine#43079) 2023-06-22 [email protected] Roll Skia from 5013b651f8ec to 117f57a53215 (1 revision) (flutter/engine#43078) 2023-06-22 [email protected] Roll Fuchsia Mac SDK from QtQznuUmHMTyORqxJ... to Ylc35wOk0_j0NLzDv... (flutter/engine#43076) 2023-06-22 [email protected] Roll ANGLE from a2b3f9b64670 to 7658525166a4 (1 revision) (flutter/engine#43075) 2023-06-22 [email protected] Roll ANGLE from ac263582dda4 to a2b3f9b64670 (1 revision) (flutter/engine#43074) 2023-06-22 [email protected] Roll Skia from 71047dca9f77 to 5013b651f8ec (4 revisions) (flutter/engine#43073) 2023-06-22 [email protected] [Impeller] Correctly compute UVs in texture fill (flutter/engine#43028) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from 7EZeNE4aGd29 to tcVndpnH_jzG fuchsia/sdk/core/mac-amd64 from QtQznuUmHMTy to Ylc35wOk0_j0 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
2 participants
At least from what I've found online, this API is already safe to call from multiple threads. From testing, at startup the presence of the mutex is adding ~100ms to shader creation time, as all compilations across N threads are fighting for the same lock.
flutter/flutter#129050