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

[dnfield]

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.

[dnfield]

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

[gaaclarke]

Please capture a unique_ptr instead of a raw pointer.

[dnfield]

I can't, because mapping is not copiable.

[gaaclarke]

You can manually copy them (as noted above).

[dnfield]

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.

[gaaclarke]

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.

[gaaclarke]

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

[dnfield]

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]

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]

@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]

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 👍

[gaaclarke]

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.

[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]

Updated the test to not need to abort.