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
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
454e2df
Enable lazy-async-stacks by-default in all modes
mkustermann Feb 12, 2020
94cf1cf
Remove settings_.default_flags in shell/platform/fuchsia/flutter/comp…
mkustermann Feb 13, 2020
3517041
Update ShellTest_WhitelistedDartVMFlag to check for the two flags
mkustermann Feb 13, 2020
70a817e
Ensure ./ci/format.sh works
mkustermann Feb 19, 2020
706b09c
empty commit to re-trigger presubmit checks
mkustermann Feb 20, 2020
34179f2
merge 'origin/master'
mkustermann Feb 20, 2020
14e5334
empty commit to re-trigger presubmit checks
mkustermann Feb 20, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion runtime/dart_vm.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,8 @@ static const char* kDartLanguageArgs[] = {
// clang-format off
"--enable_mirrors=false",
"--background_compilation",
"--causal_async_stacks",
"--no-causal_async_stacks",
"--lazy_async_stacks",
// clang-format on
};

Expand Down
12 changes: 8 additions & 4 deletions shell/common/shell_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,16 +281,20 @@ TEST_F(ShellTest, BlacklistedDartVMFlag) {
TEST_F(ShellTest, WhitelistedDartVMFlag) {
const std::vector<fml::CommandLine::Option> options = {
fml::CommandLine::Option("dart-flags",
"--lazy_async_stacks,--no-causal_async_stacks,"
"--max_profile_depth 1,--random_seed 42")};
fml::CommandLine command_line("", options, std::vector<std::string>());
flutter::Settings settings = flutter::SettingsFromCommandLine(command_line);

EXPECT_GE(settings.dart_flags.size(), 2u);
EXPECT_EQ(settings.dart_flags[0], "--lazy_async_stacks");
EXPECT_EQ(settings.dart_flags[1], "--no-causal_async_stacks");
#if !FLUTTER_RELEASE
EXPECT_EQ(settings.dart_flags.size(), 2u);
EXPECT_EQ(settings.dart_flags[0], "--max_profile_depth 1");
EXPECT_EQ(settings.dart_flags[1], "--random_seed 42");
EXPECT_EQ(settings.dart_flags.size(), 4u);
EXPECT_EQ(settings.dart_flags[2], "--max_profile_depth 1");
EXPECT_EQ(settings.dart_flags[3], "--random_seed 42");
#else
EXPECT_EQ(settings.dart_flags.size(), 0u);
EXPECT_EQ(settings.dart_flags.size(), 2u);
#endif
}

Expand Down
2 changes: 1 addition & 1 deletion shell/common/switches.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,8 @@ struct SwitchDesc {

// clang-format off
static const std::string gDartFlagsWhitelist[] = {
"--no-causal_async_stacks",
"--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?

};
// clang-format on

Expand Down
3 changes: 1 addition & 2 deletions shell/platform/fuchsia/dart_runner/dart_runner.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,7 @@ namespace {

const char* kDartVMArgs[] = {
// clang-format off
// TODO(FL-117): Re-enable causal async stack traces when this issue is
// addressed.
"--lazy_async_stacks",
"--no_causal_async_stacks",

#if !defined(FLUTTER_PROFILE)
Expand Down
1 change: 1 addition & 0 deletions shell/platform/fuchsia/dart_runner/embedder/BUILD.gn
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ template("create_aot_snapshot") {

args = [
"--no_causal_async_stacks",
"--lazy_async_stacks",
"--deterministic",
"--snapshot_kind=vm-aot-assembly",
"--assembly=" + rebase_path(snapshot_assembly),
Expand Down
3 changes: 1 addition & 2 deletions shell/platform/fuchsia/dart_runner/kernel/BUILD.gn
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,8 @@ template("create_kernel_core_snapshot") {
tool = gen_snapshot_to_use

args = [
# TODO(FL-117): Re-enable causal async stack traces when this issue is
# addressed.
"--no_causal_async_stacks",
"--lazy_async_stacks",
"--use_bytecode_compiler",
"--enable_mirrors=false",
"--deterministic",
Expand Down
1 change: 1 addition & 0 deletions shell/platform/fuchsia/dart_runner/vmservice/BUILD.gn
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ template("aot_snapshot") {

args = [
"--no_causal_async_stacks",
"--lazy_async_stacks",
"--deterministic",
"--snapshot_kind=app-aot-elf",
"--elf=" + rebase_path(snapshot_path),
Expand Down
4 changes: 0 additions & 4 deletions shell/platform/fuchsia/flutter/component.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,10 +353,6 @@ Application::Application(
settings_.task_observer_remove = std::bind(
&CurrentMessageLoopRemoveAfterTaskObserver, std::placeholders::_1);

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

// Disable code collection as it interferes with JIT code warmup
// by decreasing usage counters and flushing code which is still useful.
settings_.dart_flags.push_back("--no-collect_code");
Expand Down
3 changes: 1 addition & 2 deletions shell/platform/fuchsia/flutter/kernel/BUILD.gn
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,9 +75,8 @@ template("core_snapshot") {
tool = gen_snapshot_to_use

args = [
# TODO(FL-117): Re-enable causal async stack traces when this issue is
# addressed.
"--no_causal_async_stacks",
"--lazy_async_stacks",
"--use_bytecode_compiler",
"--enable_mirrors=false",
"--deterministic",
Expand Down
3 changes: 2 additions & 1 deletion testing/scenario_app/compile_ios_jit.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@ echo "Compiling JIT Snapshot..."

"$DEVICE_TOOLS/gen_snapshot" --deterministic \
--enable-asserts \
--causal_async_stacks \
--no-causal_async_stacks \
--lazy_async_stacks \
--isolate_snapshot_instructions="$OUTDIR/isolate_snapshot_instr" \
--snapshot_kind=app-jit \
--load_vm_snapshot_data="$DEVICE_TOOLS/../gen/flutter/lib/snapshot/vm_isolate_snapshot.bin" \
Expand Down
3 changes: 2 additions & 1 deletion testing/testing.gni
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,8 @@ template("dart_snapshot_aot") {
]

args = [
"--causal_async_stacks",
"--no-causal_async_stacks",
"--lazy_async_stacks",
"--deterministic",
"--snapshot_kind=app-aot-elf",
"--elf=" + rebase_path(elf_object),
Expand Down