[Impeller] Remove cached pipelines when RuntimeStage is hot reloaded #37307
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. |
| it = pipelines_.erase(it); | ||
| continue; | ||
| } | ||
| it++; |
There was a problem hiding this comment.
Why here and not as the last clause of the for?
There was a problem hiding this comment.
The continue would make them not identical.
| it = pipelines_.erase(it); | ||
| continue; | ||
| } | ||
| it++; |
There was a problem hiding this comment.
The continue would make them not identical.
| // |PipelineLibrary| | ||
| void PipelineLibraryGLES::RemovePipelinesWithEntryPoint( | ||
| std::shared_ptr<const ShaderFunction> function) { | ||
| for (auto it = pipelines_.begin(); it != pipelines_.end();) { |
There was a problem hiding this comment.
I'm never quite sure about iterator invalidation rules due to mutation while enumeration and the safer C++20 std::erase_if is unfortunately unavailable to us. Can we put an erase_if shim in //impeller/base somewhere and do the dumb thing for now where do do mutate while enumerate (separate allocations are OK). This may be safe as written but it is hard to read and brittle in case we change the container type.
There was a problem hiding this comment.
Will also dry up the code a lot.
There was a problem hiding this comment.
Done. LMK if this is more or less what you had in mind.
bdero
force-pushed
the
bdero/pipeline-remove
branch
2 times, most recently
from
4ce5b35 to
0c44636
Compare
|
Gold has detected about 103 new digest(s) on patchset 7. |
auto-submit
bot
removed
the
autosubmit
|
auto label is removed for flutter/engine, pr: 37307, due to - The status or check suite Windows Android AOT Engine has failed. Please fix the issues identified (or deflake) before re-applying this label.
|
bdero
force-pushed
the
bdero/pipeline-remove
branch
from
f25f9b1 to
f9fd352
Compare
|
Gold has detected about 103 new digest(s) on patchset 9. |
|
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. |
…114732) * aeae6afb7 Roll Dart SDK from b2aad7caafc9 to d97d5ad98893 (1 revision) (flutter/engine#37333) * 396be860f [Impeller] Remove cached pipelines when RuntimeStage is hot reloaded (flutter/engine#37307) * 223a485ce Handle include paths the same way as output paths (flutter/engine#37335)
…lutter#114732) * aeae6afb7 Roll Dart SDK from b2aad7caafc9 to d97d5ad98893 (1 revision) (flutter/engine#37333) * 396be860f [Impeller] Remove cached pipelines when RuntimeStage is hot reloaded (flutter/engine#37307) * 223a485ce Handle include paths the same way as output paths (flutter/engine#37335)
…lutter#114732) * aeae6afb7 Roll Dart SDK from b2aad7caafc9 to d97d5ad98893 (1 revision) (flutter/engine#37333) * 396be860f [Impeller] Remove cached pipelines when RuntimeStage is hot reloaded (flutter/engine#37307) * 223a485ce Handle include paths the same way as output paths (flutter/engine#37335)
This is the last missing bit on Impeller's end to get shader hot reload working!
PXL_20221104_040627122.mp4