Avoid reloading the kernel snapshot when spawning an isolate in the same group #48478

dnfield · 2023-11-29T00:27:46Z

Found by @mraleph while looking at flutter_tester related things with me.

I took some suggestions from him and added a test. This should eventually help speed up running large multi-file test suites.

The test is a little bit wonky because we don't have great inspection points (e.g. something to mock or examine from a test). It should break if we change the way we load kernel assets in the test harness, and is verifying that we only ask for the test asset once where we used to ask for it twice.

…ame group

dnfield · 2023-11-29T00:28:09Z

(if @mraleph or @aam know a better way to ask via Dart API whether the kernel got reloaded that would be even better)

gaaclarke

Mostly looks good. The big thing is that it should be named to match its semantics and it needs an integration test (and implementation) that kExistingGroup is used when spawning a new engine to say that the issue is fixed. Right now I think you'll get the default value which is kNewGroup.

runtime/dart_isolate.cc

runtime/dart_isolate.h

runtime/isolate_configuration.cc

runtime/dart_isolate_unittests.cc

gaaclarke · 2023-11-29T00:38:55Z

runtime/dart_isolate_unittests.cc

+ // DartIsolate have virtual methods so it can be mocked or exposing weird
+ // test-only API on IsolateConfiguration.
+ settings.application_kernels =
+ [&get_kernel_count, mapping = mappings.front().release()]() -> Mappings {


Please capture a unique_ptr instead of a raw pointer.

I can't, because mapping is not copiable.

You can manually copy them (as noted above).

I do not want to do this in this patch. The fixtures are set up to do file mappings, and if I do something like that here it will be one lone place where the fixture loading has to change.

The whole point of this test is that loading them twice shouldn't happen. I've updated the test so that it will abort instead of segfaulting in this case.

Okay, that's fine. You can still make this conform to the google style guide by capturing the unique_ptr and making your closure mutable.

settings.application_kernels = [&get_kernel_count, mapping = std::move(mappings.front())]() mutable -> Mappings { ASSERT_TRUE(mapping); // <-- This assurt will fail if it's called twice. get_kernel_count ; if (get_kernel_count > 1) { FML_LOG(ERROR) << "Unexpectedly got more than one call for the kernel mapping."; abort(); } std::vector<std::unique_ptr<const fml::Mapping>> kernel_mappings; std::unique_ptr<const fml::Mapping> emplace_mapping; mapping.swap(emplace_mapping); kernel_mappings.emplace_back(std::move(emplace_mapping)); return kernel_mappings; };

You might have to make the closure copiable. I can't remember those rules off the top of my head.

edit: updated to use a swap to be safe.

Actually, that's not quite safe. You have to use a std::swap or set it to null after the move, sorry.

runtime/dart_isolate_unittests.cc

runtime/dart_isolate.h

dnfield · 2023-11-29T15:41:07Z

Mostly looks good. The big thing is that it should be named to match its semantics and it needs an integration test (and implementation) that kExistingGroup is used when spawning a new engine to say that the issue is fixed. Right now I think you'll get the default value which is kNewGroup.

I realized while working on this that what I told you offline wasn't quite right.

This change only affects JIT mode when kernel is used - which is how flutter_tester works, and how flutter in debug mode works, but not how flutter in AOT mode works. I think we can address whether it's worth using this in JIT mode for debug builds that use this feature separately.

dnfield · 2023-11-29T15:44:44Z

And I know it's probably not entirely clear, but if the kExistingGroup stuff wasn't being respected in the test, the test I added would fail - the isolate configuration would try to grab the mapping pointer again instead of ignoring it.

dnfield · 2023-11-29T16:18:43Z

@aam pointed out that the term Root isolate is a bit confusing at this point, filed flutter/flutter#139239 to track cleaning that up.

dnfield · 2023-11-30T18:58:00Z

This uncovered a bug where we leak isolate group data for spawned isolates.

gaaclarke

LGTM modulo the raw pointer. We just need to get rid of the raw pointer to conform with the style guide. I missed the part in dart_isolate.cc so the integration I asked for previously is unnecessary. This test covers everything 👍

runtime/dart_isolate.h

gaaclarke · 2023-11-30T20:47:27Z

runtime/dart_isolate_unittests.cc

+ // DartIsolate have virtual methods so it can be mocked or exposing weird
+ // test-only API on IsolateConfiguration.
+ settings.application_kernels =
+ [&get_kernel_count, mapping = mappings.front().release()]() -> Mappings {


Okay, that's fine. You can still make this conform to the google style guide by capturing the unique_ptr and making your closure mutable.

settings.application_kernels = [&get_kernel_count, mapping = std::move(mappings.front())]() mutable -> Mappings { ASSERT_TRUE(mapping); // <-- This assurt will fail if it's called twice. get_kernel_count ; if (get_kernel_count > 1) { FML_LOG(ERROR) << "Unexpectedly got more than one call for the kernel mapping."; abort(); } std::vector<std::unique_ptr<const fml::Mapping>> kernel_mappings; std::unique_ptr<const fml::Mapping> emplace_mapping; mapping.swap(emplace_mapping); kernel_mappings.emplace_back(std::move(emplace_mapping)); return kernel_mappings; };

You might have to make the closure copiable. I can't remember those rules off the top of my head.

edit: updated to use a swap to be safe.

Co-authored-by: gaaclarke <30870216 [email protected]>

gaaclarke

LGTM. I would prefer that the failed test wouldn't take down the whole test runner. If that's hard to do that's fine as long as the crash is obvious what the problem is.

dnfield · 2023-11-30T22:27:47Z

Updated the test to not need to abort.

…e in the same group (flutter/engine#48478)

…139342) flutter/engine@74d2df5...c26e6ce 2023-12-01 [email protected] Roll a new version of googletest (2021->2023). (flutter/engine#48285) 2023-12-01 1961493 [email protected] [canvaskit] Add ImageFilter.compose (flutter/engine#48546) 2023-12-01 [email protected] Avoid reloading the kernel snapshot when spawning an isolate in the same group (flutter/engine#48478) 2023-12-01 43054281 [email protected] [Android] Check for text to paste before trying to retrieve data from URI (flutter/engine#48166) 2023-11-30 [email protected] Roll Skia from 88a1bcb9e43e to 0a90c366ff87 (3 revisions) (flutter/engine#48549) 2023-11-30 [email protected] Manual roll Dart SDK to 3.3.0-174.2.beta (4 revisions) (flutter/engine#48538) 2023-11-30 [email protected] Roll Skia from db3399a541f3 to 88a1bcb9e43e (1 revision) (flutter/engine#48545) 2023-11-30 737941 [email protected] [Windows] Begin decoupling text input plugin from the view (flutter/engine#47833) 2023-11-30 [email protected] Roll Skia from 2d236de89898 to db3399a541f3 (3 revisions) (flutter/engine#48543) 2023-11-30 [email protected] [Impeller] Add direct tesselation of circles for DrawCircle and Round end caps (flutter/engine#48103) 2023-11-30 1961493 [email protected] [canvaskit] Revert to `drawImage` rendering on Chrome 110 or earlier (flutter/engine#48515) 2023-11-30 [email protected] Roll Skia from 6b4bdebaab88 to 2d236de89898 (1 revision) (flutter/engine#48537) 2023-11-30 30870216 [email protected] [Impeller] Started expanding the blur clip region (flutter/engine#48535) 2023-11-30 [email protected] Roll Skia from 0e479728cc1f to 6b4bdebaab88 (1 revision) (flutter/engine#48536) 2023-11-30 [email protected] Roll Skia from 0968fe18ff75 to 0e479728cc1f (1 revision) (flutter/engine#48534) 2023-11-30 103135467 [email protected] Use Chromium mirror for archive dependency (flutter/engine#48509) 2023-11-30 [email protected] Roll Skia from 2f01d500a352 to 0968fe18ff75 (2 revisions) (flutter/engine#48531) 2023-11-30 [email protected] Roll Skia from 11a15444a3aa to 2f01d500a352 (1 revision) (flutter/engine#48524) 2023-11-30 [email protected] Roll Skia from 84fdd36b1eea to 11a15444a3aa (1 revision) (flutter/engine#48523) 2023-11-30 [email protected] [Android] Add support for the PlatformChannel "Share.invoke" command (flutter/engine#48265) 2023-11-30 [email protected] Roll Skia from 5d64b1322879 to 84fdd36b1eea (1 revision) (flutter/engine#48521) 2023-11-30 [email protected] Roll Skia from 23721750e433 to 5d64b1322879 (1 revision) (flutter/engine#48520) 2023-11-30 [email protected] Roll Fuchsia Linux SDK from 8wu5EgBh1yJPNOd5W... to Bb2k375udWIltCEAx... (flutter/engine#48519) 2023-11-30 [email protected] Roll Skia from 5a635f2211ce to 23721750e433 (1 revision) (flutter/engine#48514) 2023-11-29 [email protected] Roll Skia from 928e8950e8e3 to 5a635f2211ce (1 revision) (flutter/engine#48511) 2023-11-29 [email protected] [web] No implicit view in multi-view mode (flutter/engine#48505) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from 8wu5EgBh1yJP to Bb2k375udWIl 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],[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

…lutter#139342) flutter/engine@74d2df5...c26e6ce 2023-12-01 [email protected] Roll a new version of googletest (2021->2023). (flutter/engine#48285) 2023-12-01 1961493 [email protected] [canvaskit] Add ImageFilter.compose (flutter/engine#48546) 2023-12-01 [email protected] Avoid reloading the kernel snapshot when spawning an isolate in the same group (flutter/engine#48478) 2023-12-01 43054281 [email protected] [Android] Check for text to paste before trying to retrieve data from URI (flutter/engine#48166) 2023-11-30 [email protected] Roll Skia from 88a1bcb9e43e to 0a90c366ff87 (3 revisions) (flutter/engine#48549) 2023-11-30 [email protected] Manual roll Dart SDK to 3.3.0-174.2.beta (4 revisions) (flutter/engine#48538) 2023-11-30 [email protected] Roll Skia from db3399a541f3 to 88a1bcb9e43e (1 revision) (flutter/engine#48545) 2023-11-30 737941 [email protected] [Windows] Begin decoupling text input plugin from the view (flutter/engine#47833) 2023-11-30 [email protected] Roll Skia from 2d236de89898 to db3399a541f3 (3 revisions) (flutter/engine#48543) 2023-11-30 [email protected] [Impeller] Add direct tesselation of circles for DrawCircle and Round end caps (flutter/engine#48103) 2023-11-30 1961493 [email protected] [canvaskit] Revert to `drawImage` rendering on Chrome 110 or earlier (flutter/engine#48515) 2023-11-30 [email protected] Roll Skia from 6b4bdebaab88 to 2d236de89898 (1 revision) (flutter/engine#48537) 2023-11-30 30870216 [email protected] [Impeller] Started expanding the blur clip region (flutter/engine#48535) 2023-11-30 [email protected] Roll Skia from 0e479728cc1f to 6b4bdebaab88 (1 revision) (flutter/engine#48536) 2023-11-30 [email protected] Roll Skia from 0968fe18ff75 to 0e479728cc1f (1 revision) (flutter/engine#48534) 2023-11-30 103135467 [email protected] Use Chromium mirror for archive dependency (flutter/engine#48509) 2023-11-30 [email protected] Roll Skia from 2f01d500a352 to 0968fe18ff75 (2 revisions) (flutter/engine#48531) 2023-11-30 [email protected] Roll Skia from 11a15444a3aa to 2f01d500a352 (1 revision) (flutter/engine#48524) 2023-11-30 [email protected] Roll Skia from 84fdd36b1eea to 11a15444a3aa (1 revision) (flutter/engine#48523) 2023-11-30 [email protected] [Android] Add support for the PlatformChannel "Share.invoke" command (flutter/engine#48265) 2023-11-30 [email protected] Roll Skia from 5d64b1322879 to 84fdd36b1eea (1 revision) (flutter/engine#48521) 2023-11-30 [email protected] Roll Skia from 23721750e433 to 5d64b1322879 (1 revision) (flutter/engine#48520) 2023-11-30 [email protected] Roll Fuchsia Linux SDK from 8wu5EgBh1yJPNOd5W... to Bb2k375udWIltCEAx... (flutter/engine#48519) 2023-11-30 [email protected] Roll Skia from 5a635f2211ce to 23721750e433 (1 revision) (flutter/engine#48514) 2023-11-29 [email protected] Roll Skia from 928e8950e8e3 to 5a635f2211ce (1 revision) (flutter/engine#48511) 2023-11-29 [email protected] [web] No implicit view in multi-view mode (flutter/engine#48505) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from 8wu5EgBh1yJP to Bb2k375udWIl 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],[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

Avoid reloading the kernel snapshot when spawning an isolate in the s…

2ac3196

…ame group

dnfield requested review from mraleph, aam and gaaclarke November 29, 2023 00:27

gaaclarke requested changes Nov 29, 2023

View reviewed changes

Aaron review

7496e88

dnfield added 2 commits November 29, 2023 07:54

missing file

8c6ab06

fix condition

24811e3

dnfield mentioned this pull request Nov 30, 2023

VM calls embedder supplied isolate group cleanup callback when the vm-isolate shuts down dart-lang/sdk#54199

Closed

dnfield added 2 commits November 30, 2023 11:03

avoid leaking isolate group data

c9ac2c5

lint fix

f202786

dnfield requested a review from gaaclarke November 30, 2023 20:29

gaaclarke reviewed Nov 30, 2023

View reviewed changes

dnfield and others added 2 commits November 30, 2023 13:23

Update runtime/dart_isolate.h

1b95db1

Co-authored-by: gaaclarke <30870216 [email protected]>

unique_ptr copiable lambda

5c912bf

gaaclarke approved these changes Nov 30, 2023

View reviewed changes

less crashy

5b20ee0

dnfield added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 1, 2023

auto-submit bot merged commit 0895dee into flutter:main Dec 1, 2023
27 checks passed

dnfield deleted the group branch December 1, 2023 00:36

engine-flutter-autoroll mentioned this pull request Dec 1, 2023

Roll Flutter Engine from 74d2df52514a to 63484b316d78 (25 revisions) flutter/flutter#139340

Closed

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

0895deef6 Avoid reloading the kernel snapshot when spawning an isolat…

4b7b374

…e in the same group (flutter/engine#48478)

engine-flutter-autoroll mentioned this pull request Dec 1, 2023

Roll Flutter Engine from 74d2df52514a to c26e6ced11df (26 revisions) flutter/flutter#139342

Merged

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

0895deef6 Avoid reloading the kernel snapshot when spawning an isolat…

7bd4cf5

…e in the same group (flutter/engine#48478)

jiahaog mentioned this pull request Dec 4, 2023

Add flutter prefix to import jiahaog/flutter-engine#42

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid reloading the kernel snapshot when spawning an isolate in the same group #48478

Avoid reloading the kernel snapshot when spawning an isolate in the same group #48478

dnfield commented Nov 29, 2023

dnfield commented Nov 29, 2023

gaaclarke left a comment

gaaclarke Nov 29, 2023

dnfield Nov 29, 2023

gaaclarke Nov 29, 2023

dnfield Nov 30, 2023

gaaclarke Nov 30, 2023 •

edited

Loading

gaaclarke Nov 30, 2023

dnfield commented Nov 29, 2023

dnfield commented Nov 29, 2023

dnfield commented Nov 29, 2023

dnfield commented Nov 30, 2023

gaaclarke left a comment

gaaclarke Nov 30, 2023 •

edited

Loading

gaaclarke left a comment

dnfield commented Nov 30, 2023

Avoid reloading the kernel snapshot when spawning an isolate in the same group #48478

Avoid reloading the kernel snapshot when spawning an isolate in the same group #48478

Conversation

dnfield commented Nov 29, 2023

dnfield commented Nov 29, 2023

gaaclarke left a comment

Choose a reason for hiding this comment

gaaclarke Nov 29, 2023

Choose a reason for hiding this comment

dnfield Nov 29, 2023

Choose a reason for hiding this comment

gaaclarke Nov 29, 2023

Choose a reason for hiding this comment

dnfield Nov 30, 2023

Choose a reason for hiding this comment

gaaclarke Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

gaaclarke Nov 30, 2023

Choose a reason for hiding this comment

dnfield commented Nov 29, 2023

dnfield commented Nov 29, 2023

dnfield commented Nov 29, 2023

dnfield commented Nov 30, 2023

gaaclarke left a comment

Choose a reason for hiding this comment

gaaclarke Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

gaaclarke left a comment

Choose a reason for hiding this comment

dnfield commented Nov 30, 2023

gaaclarke Nov 30, 2023 •

edited

Loading

gaaclarke Nov 30, 2023 •

edited

Loading