Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Enable lazy-async-stacks by-default in all modes #16556

Merged
merged 7 commits into from
Feb 20, 2020

Conversation

mkustermann
Copy link
Member

This was already enabled by-default in AOT mode in [0] - which made the
gen_snapshot invocations use "--lazy-async-stacks --no-causal-async-stacks".

This change does the same with the engine defaults, which makes this be enabled
by-default in JIT mode as well.

See go/dart-10x-faster-async for more information.

[0] flutter/flutter@347823234fd

This was already enabled by-default in AOT mode in [0] - which made the
gen_snapshot invocations use "--lazy-async-stacks
--no-causal-async-stacks".

See go/dart-10x-faster-async for more information.

[0] flutter/flutter@347823234fd
"--lazy_async_stacks",
"--no-causal_async_stacks",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is now already this default, putting it in the whitelist doesn't make sense anymore.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Dart whitelist can be empty in release mode now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To prevent breaking workflows that specify the flag that was in the whitelist but isn't anymore because it is the default, you'll need to patch IsWhitelistedDartVMFlag to not terminate VM launch if a non-whitelisted but default flag is specified by the embedder.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a test to runtime_unittests (specifically dart_vm_unitttests.cc) that verifies whitelists. Those work in AOT as well as JIT.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is now already this default, putting it in the whitelist doesn't make sense anymore.
The Dart whitelist can be empty in release mode now

Some flutter downstream customers might explicitly specify them, which is why I left it here.

To prevent breaking workflows that specify the flag that was in the whitelist but isn't anymore because it is the default,
you'll need to patch IsWhitelistedDartVMFlag to not terminate VM launch if a non-whitelisted but default flag is
specified by the embedder.

Doesn't this have the same effect as just leaving it in the whitelist?

This is only temporary anyway, the plan is as follows:

Step 1) Enable it in JIT and AOT by-default in the engine & roll it through (this PR)
Step 2) Change the Dart VM defaults & roll it through (cannot do this right now, because embedders need to explicitly specify both flags for that to work)
Step 3) Remove both flags in all places (g3, flutter, dart vm)

I plan on doing these three steps in the next 3 weeks to give each change some time to roll.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this have the same effect as just leaving it in the whitelist?

Right. It was just a suggestion for cleaning up the code a bit for readability. I was wondering why the flag was specified in a whitelist if it was already applied by default. Folks might try to "clean it up" in the future and break exiting apps. Hence my suggestion to just make the whitelist check just ignore the default flags and reject additional flags not in the whitelist.

If this is temporary, go for it.

Step 3) Remove both flags in all places (g3, flutter, dart vm)

This still leaves custom embedders susceptible to having VM launches terminated by the flags not in the whitelist (but present by default). For this reason, I believe updating the whitelist check is a less invasive way to go.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still leaves custom embedders susceptible to having VM launches terminated by the flags not in the whitelist (but
present by default). For this reason, I believe updating the whitelist check is a less invasive way to go.

Thank you for explaining, that makes sense. I can do that change when I remove the explicit passing of the flags in Step 3 (which will first be done in the engine, only later on we'll remove the flag support from the VM itself). Does that sound ok?

// TODO(FL-117): Re-enable causal async stack traces when this issue is
// addressed.
settings_.dart_flags = {"--no_causal_async_stacks"};
settings_.dart_flags = {"--no_causal_async_stacks", "--lazy_async_stacks"};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are already in the defaults, so this entire setting specification can go away.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@jonahwilliams
Copy link
Member

Are the changes in async stacks worth the performance gain here? For debug mode we specifically don't make any performance guarantees - unless this would be for something like the jit_product runner

Copy link
Member Author

@mkustermann mkustermann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @chinmaygarde for the first look.

// TODO(FL-117): Re-enable causal async stack traces when this issue is
// addressed.
settings_.dart_flags = {"--no_causal_async_stacks"};
settings_.dart_flags = {"--no_causal_async_stacks", "--lazy_async_stacks"};
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

"--lazy_async_stacks",
"--no-causal_async_stacks",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is now already this default, putting it in the whitelist doesn't make sense anymore.
The Dart whitelist can be empty in release mode now

Some flutter downstream customers might explicitly specify them, which is why I left it here.

To prevent breaking workflows that specify the flag that was in the whitelist but isn't anymore because it is the default,
you'll need to patch IsWhitelistedDartVMFlag to not terminate VM launch if a non-whitelisted but default flag is
specified by the embedder.

Doesn't this have the same effect as just leaving it in the whitelist?

This is only temporary anyway, the plan is as follows:

Step 1) Enable it in JIT and AOT by-default in the engine & roll it through (this PR)
Step 2) Change the Dart VM defaults & roll it through (cannot do this right now, because embedders need to explicitly specify both flags for that to work)
Step 3) Remove both flags in all places (g3, flutter, dart vm)

I plan on doing these three steps in the next 3 weeks to give each change some time to roll.

@mkustermann
Copy link
Member Author

Are the changes in async stacks worth the performance gain here? For debug mode we specifically
don't make any performance guarantees - unless this would be for something like the jit_product
runner

Developers are naturally interested in having the application behavior during development be as close as possible to the behavior in production.

  • That includes the semantics (in this case: the stringification of stack traces). Imagine developers writing tests expecting the stack traces to look a certain way: We want those tests to work in flutter-debug as well as in flutter-profile:

  • It also includes performance to a certain extend. If the app is super janky during development then a) it is a bad developer experience and b) developers need to constantly build & test the flutter-profile version to ensure there's no jank there.

Which stack trace format is semantically better is also up to interpretation: We have received feedback from some users saying that --lazy-stack-traces is better (since it shows where a Future is await'ed, whereas --causal-async-stacks effectively shows where the Future was allocated).

The VM team has a strong interest in having consistent behavior as well as high performance for our customers.

Based on the feedback received on go/dart-10x-faster-async (LGTM from Ian) it seems like we can move forward to making this the only mode.

@mkustermann
Copy link
Member Author

/cc @a-siva

Copy link
Member

@chinmaygarde chinmaygarde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you file a bug for adding a test that asserts whitelist behavior please? I'll follow up on it.

"--lazy_async_stacks",
"--no-causal_async_stacks",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this have the same effect as just leaving it in the whitelist?

Right. It was just a suggestion for cleaning up the code a bit for readability. I was wondering why the flag was specified in a whitelist if it was already applied by default. Folks might try to "clean it up" in the future and break exiting apps. Hence my suggestion to just make the whitelist check just ignore the default flags and reject additional flags not in the whitelist.

If this is temporary, go for it.

Step 3) Remove both flags in all places (g3, flutter, dart vm)

This still leaves custom embedders susceptible to having VM launches terminated by the flags not in the whitelist (but present by default). For this reason, I believe updating the whitelist check is a less invasive way to go.

@jonahwilliams
Copy link
Member

@mkustermann that seems reasonable to me

Copy link
Member

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mkustermann
Copy link
Member Author

Thanks, @chinmaygarde

Can you file a bug for adding a test that asserts whitelist behavior please? I'll follow up on it.

There are seem to be already tests for the white list in shell/common/shell_unittests.cc. I've added the two flags to that test. Do you think this is enough or did you have something else in mind?

After switching the VM defaults, I will remove the explicit passing of the flags in the engine and can make the change to allow embedders to pass it but ignore the flags internally (instead of passing them to the VM, which will remove support eventually). Does that sound ok?

The accompanying change for flutter/flutter, which updates expectations is available in : flutter/flutter#50760

@chinmaygarde
Copy link
Member

That works as well. Thanks.

@mkustermann
Copy link
Member Author

The framework failures are expected and will be addressed when rolling this change into flutter/flutter (see flutter/flutter#50760)

The fuchsia build seems to be failing due to existing infra failures.

So this change is ready to land.

@mkustermann
Copy link
Member Author

The fuchsia builder seems to have a flaky infra failure. So I'm gonna land it now, hope that's ok

@mkustermann mkustermann merged commit fdabcad into flutter:master Feb 20, 2020
mkustermann added a commit to mkustermann/flutter that referenced this pull request Feb 20, 2020
This is the accompaning change for [0], once that change gets rolled into flutter/flutter

[0] flutter/engine#16556
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 20, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 24, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 24, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 24, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 24, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 24, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 24, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 24, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 24, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 25, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 25, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 25, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 25, 2020
dnfield added a commit that referenced this pull request Feb 25, 2020
dnfield added a commit that referenced this pull request Feb 25, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 25, 2020
dnfield pushed a commit to flutter/flutter that referenced this pull request Feb 25, 2020
* 5b0cbbe Roll fuchsia/sdk/core/mac-amd64 from _jvYk... to WZgbp... (flutter/engine#16692)

* cbb0ff8 Roll src/third_party/skia c5ff41f2976e..7dfb46e7f397 (20 commits) (flutter/engine#16691)

* ab454ea Roll src/third_party/dart 0f141be8bd52..7469b87b042a (9 commits) (flutter/engine#16693)

* 1b73784 fix param (flutter/engine#16694)

* bdd2cf8 Roll src/third_party/skia 7dfb46e7f397..9baef3593c3c (3 commits) (flutter/engine#16696)

* 8f0bbfa Roll src/third_party/skia 9baef3593c3c..ed1ff23c2768 (5 commits) (flutter/engine#16699)

* f052d2e Roll fuchsia/sdk/core/linux-amd64 from VHyDa... to YPr0t... (flutter/engine#16701)

* 3fe3325 Roll src/third_party/dart 7469b87b042a..e187e42593e8 (11 commits) (flutter/engine#16702)

* f83092a Roll src/third_party/skia ed1ff23c2768..a5097354217b (1 commits) (flutter/engine#16703)

* 6b3d439 Roll src/third_party/skia a5097354217b..2c2db2762809 (1 commits) (flutter/engine#16704)

* fdabcad Enable lazy-async-stacks by-default in all modes (flutter/engine#16556)

* 02aa865 Fix the newline on some keyboards (flutter/engine#16560)

* cde7d47 Roll src/third_party/dart e187e42593e8..81d4cc6bc99a (3 commits) (flutter/engine#16705)

* 14a2b03 Roll fuchsia/sdk/core/mac-amd64 from WZgbp... to 78ZcV... (flutter/engine#16706)

* 7b0453e Roll src/third_party/skia 2c2db2762809..9d4e31d6cda5 (1 commits) (flutter/engine#16707)

* 5cef6d0 Roll fuchsia/sdk/core/linux-amd64 from YPr0t... to CZTpy... (flutter/engine#16708)

* 0ef67b5 opt out dart:ui from nnbd (flutter/engine#16473)

* bc4a27f [shell tests] Integrate Vulkan with Shell Tests (flutter/engine#16621)

* 2fdba52 Roll src/third_party/skia 9d4e31d6cda5..62076977a0b7 (11 commits) (flutter/engine#16710)

* 969cfc1 Roll src/third_party/skia 62076977a0b7..1d589a578ca4 (6 commits) (flutter/engine#16712)

* 8eb727e Flush the SkCanvas when submitting a frame in ShellTestPlatformViewVulkan::OffscreenSurface (flutter/engine#16717)

* e44f99b Roll src/third_party/skia 1d589a578ca4..706851dc99d9 (2 commits) (flutter/engine#16714)

* 60b27fd Reland "Remove usage of Dart_AllocateWithNativeFields" (flutter/engine#16713)

* 7a50e4d Roll src/third_party/dart 81d4cc6bc99a..fd20c7b92bb8 (31 commits) (flutter/engine#16716)

* 4aaafe0 Roll src/third_party/skia 706851dc99d9..df283d01cabb (3 commits) (flutter/engine#16719)

* e18aba3 Refactor of ClaimDartHandle -> AssociateWithDartWrapper (flutter/engine#16720)

* 66742b6 Roll src/third_party/skia df283d01cabb..3ffa7af75301 (1 commits) (flutter/engine#16722)

* e0303b0 Roll src/third_party/dart fd20c7b92bb8..6ef9131d82c4 (7 commits) (flutter/engine#16723)

* 0bd69f9 Roll src/third_party/skia 3ffa7af75301..77521d9e06e8 (2 commits) (flutter/engine#16725)

* 3e69286 Roll fuchsia/sdk/core/mac-amd64 from 78ZcV... to iYYAH... (flutter/engine#16726)

* 704396b Roll fuchsia/sdk/core/linux-amd64 from CZTpy... to -u-iU... (flutter/engine#16727)

* 645d4e6 Roll src/third_party/dart 6ef9131d82c4..5829fc7829d5 (3 commits) (flutter/engine#16728)

* b73dfed Roll src/third_party/skia 77521d9e06e8..392846665c40 (1 commits) (flutter/engine#16729)

* 2724727 Roll src/third_party/skia 392846665c40..bf5cb0f539e7 (1 commits) (flutter/engine#16730)

* ab0dd12 [web] Running safari tests on LUCI (flutter/engine#16715)

* e5091a8 Enable Vulkan-related shell unittests on Fuchsia (flutter/engine#16718)

* 930a80a Fix some compiler warnings in newer versions of Clang. (flutter/engine#16733)

* 941aee3 [web] add comment to skipped safari test (flutter/engine#16737)

* 136a057 [web] Rename LineMetrics.text to LineMetrics.displayText (flutter/engine#16734)

* afe7968 [web] Paragraph.longestLine doesn't need to check for `isSingleLine` anymore (flutter/engine#16736)

* c3f4c1a Migrate Path to AssociateWithDartWrapper (flutter/engine#16753)

* d0c2418 Add support for Increase Contrast on iOS (flutter/engine#15343)

* 6d2cbb2 Roll src/third_party/skia bf5cb0f539e7..46f5c5f08b61 (2 commits) (flutter/engine#16732)

* f758eb9 Roll fuchsia/sdk/core/linux-amd64 from -u-iU... to 3rB22... (flutter/engine#16752)

* a4a1f4f Roll fuchsia/sdk/core/mac-amd64 from iYYAH... to 5B5jq... (flutter/engine#16744)

* 8caf6d1 Roll src/third_party/skia 46f5c5f08b61..9e8f60534464 (29 commits) (flutter/engine#16754)

* 340f855 Roll fuchsia/sdk/core/mac-amd64 from 5B5jq... to g1vJn... (flutter/engine#16755)

* 9a9abb7 Roll src/third_party/skia 9e8f60534464..d1c90e10f0ca (1 commits) (flutter/engine#16757)

* 2e12cdc Roll src/third_party/skia d1c90e10f0ca..998066127e0d (1 commits) (flutter/engine#16759)

* 566cfae Roll src/third_party/skia 998066127e0d..57bc977e124c (3 commits) (flutter/engine#16762)

* 73fdff1 Roll fuchsia/sdk/core/linux-amd64 from 3rB22... to PGfiE... (flutter/engine#16763)

* ecca175 Roll fuchsia/sdk/core/mac-amd64 from g1vJn... to mcI8X... (flutter/engine#16764)

* 4d50517 Roll src/third_party/skia 57bc977e124c..cc5415a8ce36 (1 commits) (flutter/engine#16767)

* 237ddb1 Roll src/third_party/skia cc5415a8ce36..1cec4d5e3d92 (2 commits) (flutter/engine#16769)

* 8da64e0 Roll src/third_party/dart 5829fc7829d5..c75256299280 (43 commits) (flutter/engine#16770)

* 0d87160 Roll src/third_party/skia 1cec4d5e3d92..7d252302268a (2 commits) (flutter/engine#16771)

* 1aef7a4 Delete FlutterAppDelegate_Internal.h (flutter/engine#16772)

* b87bb0a [web] Fix canvas leak when dpi changes. Evict from BitmapCanvas cache under… (flutter/engine#16721)

* e0e24f5 Roll src/third_party/skia 7d252302268a..03b8ab225fd7 (8 commits) (flutter/engine#16773)

* 38dc2ea [i18n] Deprecates fuchsia.timezone.Timezone (flutter/engine#16700)

* 92abb22 Reland "Lift restriction that embedders may not trample the render thread OpenGL context in composition callbacks." (flutter/engine#16711)

* 971122b [web] Reduce the usage of unnecessary lists in pointer binding (flutter/engine#16745)

* d670059 [web] Respect maxLines when calculating boxes for a range (flutter/engine#16749)

* 5496bb4 Roll src/third_party/skia 03b8ab225fd7..659cc1c90705 (4 commits) (flutter/engine#16774)

* bad1fbe Roll src/third_party/dart c75256299280..73f6d15665a3 (9 commits) (flutter/engine#16776)

* d2e2cc9 Roll fuchsia/sdk/core/mac-amd64 from mcI8X... to O6w2L... (flutter/engine#16777)

* 888a62c Revert "Enable lazy-async-stacks by-default in all modes (#16556)" (flutter/engine#16781)
NoamDev pushed a commit to NoamDev/engine that referenced this pull request Feb 27, 2020
This was already enabled by-default in AOT mode in [0] - which made the
gen_snapshot invocations use "--lazy-async-stacks --no-causal-async-stacks".

See go/dart-10x-faster-async for more information.

[0] flutter/flutter@347823234fd
NoamDev pushed a commit to NoamDev/engine that referenced this pull request Feb 27, 2020
NoamDev added a commit to NoamDev/engine that referenced this pull request Feb 27, 2020
NoamDev added a commit to NoamDev/engine that referenced this pull request Feb 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants