[Impeller] Fix hot reload for shaders on Impeller #49739
Conversation
| @@ -745,6 +745,14 @@ class ContentContext { | |||
| const std::function<std::shared_ptr<Pipeline<PipelineDescriptor>>()>& | |||
| create_callback) const; | |||
|
|
|||
| /// Used by hot reload/hot restart to clear a cached pipeline from | |||
| /// GetCachedRuntimeEffectPipeline. | |||
| void ClearCachedRuntimeEffectPipeline( | |||
There was a problem hiding this comment.
its possible that from frame to frame you will get different values for options, which means you may not invalidate all of the pipeline descriptors associated with this shader consider this scenario:
Frame 1: Use and cache shader with Rect geometry, triangle geometry is used.
Frame 2: Hot reload, Use shader with tessellated geometry, triangle strip is used. Pipeline descripor matching shader name + triangle strip is removed.
Frame 3: Hot reload, use shader with rect geometry, old pipeline descriptor from frame 1 is recycled.
You'd need to clear out all possible pipelines for a given shader variant. Since this is a debug only feature, you could iterate the cache and evict everything that has a name match. Or switch to a two level cache.
There was a problem hiding this comment.
The shader comes in as dirty initially. That would mean evicting everything with the same name every time we got a new geometry, right?
There was a problem hiding this comment.
Its a one to many mapping of shaders to pipelines. So if the shader is dirty, then all pipelines (regardless of the ContentContextOptions must be marked dirty). You'd probably need to special case the first time you see a shader?
There was a problem hiding this comment.
If we really are worried about this case I think we need to just plain have a service extension that kills the cache. It's just hard to get that right with rendering so that the cache gets killed and some frame isn't in flight that puts the shader back into the cache before the new shader version comes through.
There was a problem hiding this comment.
Replace frame numbers with two kinds of draws in the same frame. If you don't invalidate all pipeline descriptors marching the name, then only the first will get updated when you got reload
There was a problem hiding this comment.
Yeah you're right, I have a sample program now that uses different blend mode.
Invalidating all the ones with the same name ... seems to work. Let's just do it.
There was a problem hiding this comment.
Ok, I've updated the evict method to just evict by name rather than including the options, and written some tests for it. This works locally for hot reload with a simple app where the blend modes are different on the same shader.
…141638) flutter/engine@f206573...eab7bd3 2024-01-16 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Manually bump Dart to 3.4.0-34.0.dev" (flutter/engine#49802) 2024-01-16 bdero@google.com Use the correct impeller-cmake-example mirror (flutter/engine#49791) 2024-01-16 bdero@google.com [Flutter GPU] Run unittests on CI and fix HostBuffer. (flutter/engine#49789) 2024-01-16 bdero@google.com [Flutter GPU] Fix playground shader paths. (flutter/engine#49790) 2024-01-16 737941+loic-sharma@users.noreply.github.com [Windows] Add README (flutter/engine#49779) 2024-01-16 dnfield@google.com Fix typo (flutter/engine#49725) 2024-01-16 jason-simmons@users.noreply.github.com Truncate thread names on Linux to the maximum allowed length (flutter/engine#49781) 2024-01-16 dnfield@google.com [Impeller] Fix hot reload for shaders on Impeller (flutter/engine#49739) 2024-01-16 jiahaog@users.noreply.github.com Add `flutter` prefix to import (flutter/engine#49793) 2024-01-16 ivan.inozemtsev@gmail.com Support running sound null safe kernels from dart_runner (flutter/engine#49598) 2024-01-16 bdero@google.com Manually bump Dart to 3.4.0-34.0.dev (flutter/engine#49792) 2024-01-16 49699333+dependabot[bot]@users.noreply.github.com Bump actions/upload-artifact from 3.1.3 to 4.1.0 (flutter/engine#49788) 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 jonahwilliams@google.com,rmistry@google.com,zra@google.com 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
I broke this in #49543
While investigating flutter/flutter#141235 I realized that the specific things that were wrong there were probably artifacts of some intermediate issue in the previous patch - they no longer appear. However, I had broken hot reload.
This patch fixes it and updates the
EntityTest.RuntimeEffectto artificially run the rendering callback a few times simulating a hot reload. The test fails without my changes here.