[Impeller] switch Pipeline to use raw ptr instead of shared ptr for recorded references. #57015

jonahwilliams · 2024-12-06T17:45:42Z

We don't need recorded commands to keep pipelines alive as the context does that already.

gaaclarke · 2024-12-06T18:06:47Z

impeller/entity/contents/content_context.h

@@ -377,338 +377,338 @@ class ContentContext {

  Tessellator& GetTessellator() const;

-  std::shared_ptr<Pipeline<PipelineDescriptor>> GetFastGradientPipeline(
+  const Pipeline<PipelineDescriptor>* GetFastGradientPipeline(


What do you think of wrapping this up in a raw_ptr class. That way in debug builds we could actually assert we aren't using dangling pointers.

template<typename T> class raw_ptr { public: raw_ptr(const std::shared_ptr<T>& ptr) : ptr_(ptr.get()) #if !NDEBUG , weak_ptr_(ptr) #endif {} T* get() { #if !NDEBUG FML_CHECK(weak_ptr_.lock()); #endif return ptr_; } private: T* ptr_; #if !NDEBUG std::weak_ptr<T> weak_ptr_; #endif };

I will never use the class as I never run unopt builds. I don't think we should add infrastructure for the one or two people who do.

It is executed by CI though.

so are things like ASAN

rather than writing our own ASAN code, can we just use ASAN on our tests?

I think we should be able to. There is some weirdness when executing on linux. You have to make a fake X instance. Looks like we have something in run_tests.py that does this already: https://github.com/gaaclarke/engine/blob/9fff75d9364fb3eafb62240ac1ab04f76ef914cd/testing/run_tests.py#L1220

You have to specify "impeller" as the variant though (we are just specifying engine). I don't know if it works. It might not if there are no builders executing it. I think this goes back all the way to when kaushik hooked up it.

We can try adding "impeller" to the variant in linux_unopt.json and see if it works 🤞

I tried just throwing in impeller with the asan target, it didn't work: https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8729261574606027697/+/u/test:_test:_Host_Tests_for_host_debug_unopt/stdout

The virtual display doesn't seem to be working. This is the same sort of issues I ran into when trying to get golden tests running on linux. We should have ASAN as an option on macos. That may be the easier approach.

I can't get things compiling with ASAN on macos:

$ ./flutter/tools/gn --unoptimized --runtime-mode=debug --no-rbe --no-goma --asan --mac-cpu=arm64 $ ninja -j8 -C out/host_debug_unopt_arm64 impeller_golden_tests [5/2329] ACTION //flutter/impeller/runtime_stage:flatc_runtime_stage_flatbuffers(//build/toolchain/mac:clang_arm64) FAILED: gen/flutter/impeller/runtime_stage/runtime_stage_flatbuffers.h vpython3 ../../build/gn_run_binary.py flatc --warnings-as-errors --cpp --cpp-std c++17 --cpp-static-reflection --gen-object-api --filename-suffix _flatbuffers -o gen/flutter/impeller/runtime_stage ../../flutter/impeller/runtime_stage/runtime_stage.fbs Command failed: ./flatc --warnings-as-errors --cpp --cpp-std c++17 --cpp-static-reflection --gen-object-api --filename-suffix _flatbuffers -o gen/flutter/impeller/runtime_stage ../../flutter/impeller/runtime_stage/runtime_stage.fbs exitCode: -6 dyld[61885]: missing symbol called

edit: looks like ASAN wants a runtime library and it's creating a weird link in the macho file

$ otool -L flatc flatc: @rpath/libclang_rt.asan_osx_dynamic.dylib (compatibility version 0.0.0, current version 0.0.0) /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1351.0.0)

I filed an issue for ASAN: flutter/flutter#159985

gaaclarke

Yea going this way sounds easier for now. I'd still like to get ASAN working at some point. I'll file an issue. LGTM modulo a place where I think there is a typo.

gaaclarke · 2024-12-09T17:19:14Z

impeller/entity/contents/content_context.h

-  const Pipeline<PipelineDescriptor>* GetPipeline(
-      Variants<TypedPipeline>& container,
-      ContentContextOptions opts) const {
+  PipelineRef GetPipeline(Variants<TypedPipeline>& container,


We should have a comment somewhere that says why we are able to pass around raw_ptrs. This is the best place I can think of. Or maybe at the typedef?

Done, on the typedef.

gaaclarke · 2024-12-09T17:21:28Z

impeller/entity/entity_unittests.cc

@@ -1696,7 +1697,7 @@ TEST_P(EntityTest, RuntimeEffect) {
  ASSERT_TRUE(runtime_stage->IsDirty());

  bool expect_dirty = true;
-  const Pipeline<PipelineDescriptor>* first_pipeline;
+  const raw_ptr<Pipeline<PipelineDescriptor>*> first_pipeline;


This doesn't look right, you have a pointer to a pointer here. It should be raw_ptr<PipelineDescriptor>>, no?

This is an outdated diff...

gaaclarke

lgtm, thanks! I'll continue to poke around at asan.

auto-submit · 2024-12-09T19:05:06Z

auto label is removed for flutter/engine/57015, due to - The status or check suite Linux linux_unopt has failed. Please fix the issues identified (or deflake) before re-applying this label.

Remove FML_CHECK on == so we can make comparison to nullptr.

… ptr for recorded references. (flutter/engine#57015)

…160000) flutter/engine@37a7a14...4d0e9ac 2024-12-09 [email protected] [Impeller] switch Pipeline to use raw ptr instead of shared ptr for recorded references. (flutter/engine#57015) 2024-12-09 [email protected] [Impeller] disable all Adrenos older than 630 (flutter/engine#57069) 2024-12-09 [email protected] Roll Skia to 14f8f6d984ff (flutter/engine#57068) 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] 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." (#57079) Reverts #57015 fixes flutter/flutter#160004

…tr for recorded references. (#57086) Prev: #57015 There is a unit test that is clearing out the pipeline storage, manually null out captured PipelineRef.

…ecorded references. (flutter/engine#57015) Fixes flutter#159566 We don't need recorded commands to keep pipelines alive as the context does that already.

…tr for recorded references." (flutter/engine#57079) Reverts flutter/engine#57015 fixes flutter#160004

…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.

[Impeller] ptrs baby.

3ba752d

github-actions bot added the e: impeller label Dec 6, 2024

jonahwilliams changed the title ~~[Impeller] ptrs baby.~~ [Impeller] switch Pipeline to use raw ptr instead of shared ptr for recorded references. Dec 6, 2024

jonahwilliams requested a review from gaaclarke December 6, 2024 17:46

gaaclarke reviewed Dec 6, 2024

View reviewed changes

jonahwilliams added 3 commits December 7, 2024 13:23

use raw ptr wrapper.

7ad67d7

add equality implementations to raw_ptr.

3eb58d8

licenses.

5e8c320

gaaclarke reviewed Dec 9, 2024

View reviewed changes

gaaclarke mentioned this pull request Dec 9, 2024

enable ASAN for impeller_unittests and impeller_golden_tests on macos ci flutter/flutter#159985

Open

add comment.

fb21887

gaaclarke approved these changes Dec 9, 2024

View reviewed changes

jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 9, 2024

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Dec 9, 2024

Update raw_ptr.h

43f8e05

Remove FML_CHECK on == so we can make comparison to nullptr.

jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 9, 2024

auto-submit bot merged commit 4d0e9ac into flutter:main Dec 9, 2024
33 checks passed

engine-flutter-autoroll mentioned this pull request Dec 9, 2024

Roll Flutter Engine from 37a7a144b864 to 4d0e9ac8f5ae (3 revisions) flutter/flutter#160000

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 9, 2024

4d0e9ac8f [Impeller] switch Pipeline to use raw ptr instead of shared…

dbefae7

… ptr for recorded references. (flutter/engine#57015)

This was referenced Dec 10, 2024

engine tree closed on flakey Play/EntityTest.RuntimeEffect/Metal flutter/flutter#160004

Closed

Revert "[Impeller] switch Pipeline to use raw ptr instead of shared ptr for recorded references." #57079

Merged

gaaclarke added a commit that referenced this pull request Dec 10, 2024

Revert "[Impeller] switch Pipeline to use raw ptr instead of shared p…

0338d1c

…tr for recorded references." (#57079) Reverts #57015 fixes flutter/flutter#160004

jonahwilliams mentioned this pull request Dec 10, 2024

[Impeller] reland: switch Pipeline to use raw ptr instead of shared ptr for recorded references. #57086

Merged

nick9822 pushed a commit to nick9822/flutter that referenced this pull request Dec 18, 2024

Revert "[Impeller] switch Pipeline to use raw ptr instead of shared p…

3330623

…tr for recorded references." (flutter/engine#57079) Reverts flutter/engine#57015 fixes flutter#160004

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Impeller] switch Pipeline to use raw ptr instead of shared ptr for recorded references. #57015

[Impeller] switch Pipeline to use raw ptr instead of shared ptr for recorded references. #57015

jonahwilliams commented Dec 6, 2024

gaaclarke Dec 6, 2024

jonahwilliams Dec 6, 2024

gaaclarke Dec 6, 2024

jonahwilliams Dec 6, 2024

jonahwilliams Dec 6, 2024

gaaclarke Dec 6, 2024

gaaclarke Dec 6, 2024

gaaclarke Dec 6, 2024

gaaclarke Dec 6, 2024 •

edited

Loading

gaaclarke Dec 9, 2024

gaaclarke left a comment

gaaclarke Dec 9, 2024

jonahwilliams Dec 9, 2024

gaaclarke Dec 9, 2024

jonahwilliams Dec 9, 2024

gaaclarke left a comment

auto-submit bot commented Dec 9, 2024

[Impeller] switch Pipeline to use raw ptr instead of shared ptr for recorded references. #57015

[Impeller] switch Pipeline to use raw ptr instead of shared ptr for recorded references. #57015

Conversation

jonahwilliams commented Dec 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaaclarke Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaaclarke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaaclarke left a comment

Choose a reason for hiding this comment

auto-submit bot commented Dec 9, 2024

gaaclarke Dec 6, 2024 •

edited

Loading