[Impeller] reland: switch Pipeline to use raw ptr instead of shared ptr for recorded references. #57086
Conversation
Remove FML_CHECK on == so we can make comparison to nullptr.
…to raw_ptr_pipeline
impeller/entity/entity_unittests.cc
Outdated
|
|
||
| ASSERT_TRUE(runtime_stage->IsDirty()); | ||
| expect_dirty = true; | ||
| first_pipeline = PipelineRef{nullptr}; |
There was a problem hiding this comment.
Changing this test to null out the pipeline. The test is clearing out the shader cache which invalidates this ptr.
There was a problem hiding this comment.
I'm not sure if this is the right fix. The reason being that the failure was Expected: (first_pipeline) != (pass.GetCommands().back().pipeline). It's an assertion that doesn't even look into the validity of the pointer, it just performs a computation on the address. Were you able to validate locally this removed the failure?
There was a problem hiding this comment.
I'm not sure exactly what happens here. Right now we're holding onto a raw_ptr. The structure that keeps the shared_ptr alive is being cleared out. So that raw_ptr is no longer valid.
I don't know why they do match but only sometimes.
There was a problem hiding this comment.
I'm wary of changing this test to fix the failure. I'm afraid we may be masking a bug. My intuition on this assertion is that the render pass's commands should point to a different pipeline if the runtime stage has been updated. That makes sense. This change just removes that assertion by just asserting that the pipeline is not null. So this probably removes the flakiness but not the bug it's demonstrating.
There was a problem hiding this comment.
I don't really think that this is a meaningful assertion
There was a problem hiding this comment.
Chatted offline: The most likely reason that sometimes this fails is that we get (un)lucky and we're creating a new pipeline that has the same address. Jonah's right that this is not a good assertion. I'd say that usually not equals assertions are kind of trash anyways. We should have some sort of positive assertion about the rebuild having happened.
There was a problem hiding this comment.
I can't come up with a better alternative to this assert. This is fine for me. Although I think it's better to just delete that EXPECT_NE or replace it with EXPECT_NE(nullptr, pass.GetCommands().back().pipeline.get()); instead of fiddling with state.
gaaclarke
left a comment
There was a problem hiding this comment.
Glad we came up with a probable explanation, thanks.
impeller/entity/entity_unittests.cc
Outdated
|
|
||
| ASSERT_TRUE(runtime_stage->IsDirty()); | ||
| expect_dirty = true; | ||
| first_pipeline = PipelineRef{nullptr}; |
There was a problem hiding this comment.
I can't come up with a better alternative to this assert. This is fine for me. Although I think it's better to just delete that EXPECT_NE or replace it with EXPECT_NE(nullptr, pass.GetCommands().back().pipeline.get()); instead of fiddling with state.
impeller/entity/entity_unittests.cc
Outdated
|
|
||
| if (expect_dirty) { | ||
| EXPECT_NE(first_pipeline, pass.GetCommands().back().pipeline); | ||
| first_pipeline = pass.GetCommands().back().pipeline; |
There was a problem hiding this comment.
You're going to still want this so the else condition passes. I'd remove that resetting of first_pipeline you added too.
…shared ptr for recorded references. (flutter/engine#57086)
…shared ptr for recorded references. (flutter/engine#57086)
flutter/engine@23fc43d...3a641b6 2024-12-10 robert.ancell@canonical.com Clean up key embedder responder tests (flutter/engine#57054) 2024-12-10 jonahwilliams@google.com [Impeller] reland: switch Pipeline to use raw ptr instead of shared ptr for recorded references. (flutter/engine#57086) 2024-12-10 skia-flutter-autoroll@skia.org Roll Skia from bd7d952398d5 to d541f1aa0c9b (3 revisions) (flutter/engine#57098) 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,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
…tr for recorded references. (flutter/engine#57086) Prev: flutter/engine#57015 There is a unit test that is clearing out the pipeline storage, manually null out captured PipelineRef.
Prev: #57015
There is a unit test that is clearing out the pipeline storage, manually null out captured PipelineRef.