Reland 2 (part 1): Enforce the rule of calling FlutterView.Render (#…

…47062)

This PR relands part of #45300,
which was reverted in #46919 due
to performance regression.

Due to how little and trivial production code the original PR touches, I
really couldn't figure out the exact line that caused it except through
experimentation, which requires changes to be officially landed on the
main branch. After this PR lands, I'll immediately fire a performance
test.

This PR contains the `Shell` refactor of the original PR. I made a
slight change where the isolate snapshot is no longer returned through
return value, but the parameter, in order to avoid the overhead of
assigning.

It is intentional to not contain any unit tests or other changes of the
original PR. They will be landed shortly after this PR.

Part of flutter/flutter#136826.

## Pre-launch Checklist

- [ ] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [ ] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [ ] I read and followed the [Flutter Style Guide] and the [C++,
Objective-C, Java style guides].
- [ ] I listed at least one issue that this PR fixes in the description
above.
- [ ] I added new tests to check the change I am making or feature I am
adding, or the PR is [test-exempt]. See [testing the engine] for
instructions on writing and running engine tests.
- [ ] I updated/added relevant documentation (doc comments with `///`).
- [ ] I signed the [CLA].
- [ ] All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel
on [Discord].

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#overview
[Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene
[test-exempt]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo
[C++, Objective-C, Java style guides]:
https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
[testing the engine]:
https://github.com/flutter/flutter/wiki/Testing-the-engine
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes
[Discord]: https://github.com/flutter/flutter/wiki/Chat

shell/common/shell.cc

@@ -144,31 +144,36 @@ void PerformInitializationTasks(Settings& settings) {
 
 }  // namespace
 
-std::unique_ptr<Shell> Shell::Create(
-    const PlatformData& platform_data,
-    const TaskRunners& task_runners,
-    Settings settings,
-    const Shell::CreateCallback<PlatformView>& on_create_platform_view,
-    const Shell::CreateCallback<Rasterizer>& on_create_rasterizer,
-    bool is_gpu_disabled) {
-  // This must come first as it initializes tracing.
-  PerformInitializationTasks(settings);
-
-  TRACE_EVENT0("flutter", "Shell::Create");
-
+std::pair<DartVMRef, fml::RefPtr<const DartSnapshot>>
+Shell::InferVmInitDataFromSettings(Settings& settings) {
   // Always use the `vm_snapshot` and `isolate_snapshot` provided by the
   // settings to launch the VM.  If the VM is already running, the snapshot
   // arguments are ignored.
   auto vm_snapshot = DartSnapshot::VMSnapshotFromSettings(settings);
   auto isolate_snapshot = DartSnapshot::IsolateSnapshotFromSettings(settings);
   auto vm = DartVMRef::Create(settings, vm_snapshot, isolate_snapshot);
-  FML_CHECK(vm) << "Must be able to initialize the VM.";
 
   // If the settings did not specify an `isolate_snapshot`, fall back to the
   // one the VM was launched with.
   if (!isolate_snapshot) {
     isolate_snapshot = vm->GetVMData()->GetIsolateSnapshot();
   }
+  return {std::move(vm), isolate_snapshot};
+}
+
+std::unique_ptr<Shell> Shell::Create(
+    const PlatformData& platform_data,
+    const TaskRunners& task_runners,
+    Settings settings,
+    const Shell::CreateCallback<PlatformView>& on_create_platform_view,
+    const Shell::CreateCallback<Rasterizer>& on_create_rasterizer,
+    bool is_gpu_disabled) {
+  // This must come first as it initializes tracing.
+  PerformInitializationTasks(settings);
+
+  TRACE_EVENT0("flutter", "Shell::Create");
+
+  auto [vm, isolate_snapshot] = InferVmInitDataFromSettings(settings);
   auto resource_cache_limit_calculator =
       std::make_shared<ResourceCacheLimitCalculator>(
           settings.resource_cache_max_bytes_threshold);

shell/common/shell.h

@@ -438,15 +438,29 @@ class Shell final : public PlatformView::Delegate,
   const std::shared_ptr<fml::ConcurrentTaskRunner>
   GetConcurrentWorkerTaskRunner() const;
 
+  // Infer the VM ref and the isolate snapshot based on the settings.
+  //
+  // If the VM is already running, the settings are ignored, but the returned
+  // isolate snapshot always prioritize what is specified by the settings, and
+  // falls back to the one VM was launched with.
+  //
+  // This function is what Shell::Create uses to infer snapshot settings.
+  //
+  // TODO(dkwingsmt): Extracting this method is part of a bigger change. If the
+  // entire change is not eventually landed, we should merge this method back
+  // to Create. https://github.com/flutter/flutter/issues/136826
+  static std::pair<DartVMRef, fml::RefPtr<const DartSnapshot>>
+  InferVmInitDataFromSettings(Settings& settings);
+
  private:
   using ServiceProtocolHandler =
       std::function<bool(const ServiceProtocol::Handler::ServiceProtocolMap&,
                          rapidjson::Document*)>;
 
   /// A collection of message channels (by name) that have sent at least one
-  /// message from a non-platform thread. Used to prevent printing the error log
-  /// more than once per channel, as a badly behaving plugin may send multiple
-  /// messages per second indefinitely.
+  /// message from a non-platform thread. Used to prevent printing the error
+  /// log more than once per channel, as a badly behaving plugin may send
+  /// multiple messages per second indefinitely.
   std::mutex misbehaving_message_channels_mutex_;
   std::set<std::string> misbehaving_message_channels_;
   const TaskRunners task_runners_;
@@ -497,19 +511,20 @@ class Shell final : public PlatformView::Delegate,
   bool frame_timings_report_scheduled_ = false;
 
   // Vector of FrameTiming::kCount * n timestamps for n frames whose timings
-  // have not been reported yet. Vector of ints instead of FrameTiming is stored
-  // here for easier conversions to Dart objects.
+  // have not been reported yet. Vector of ints instead of FrameTiming is
+  // stored here for easier conversions to Dart objects.
   std::vector<int64_t> unreported_timings_;
 
-  /// Manages the displays. This class is thread safe, can be accessed from any
-  /// of the threads.
+  /// Manages the displays. This class is thread safe, can be accessed from
+  /// any of the threads.
   std::unique_ptr<DisplayManager> display_manager_;
 
   // protects expected_frame_size_ which is set on platform thread and read on
   // raster thread
   std::mutex resize_mutex_;
 
-  // used to discard wrong size layer tree produced during interactive resizing
+  // used to discard wrong size layer tree produced during interactive
+  // resizing
   std::unordered_map<int64_t, SkISize> expected_frame_sizes_;
 
   // Used to communicate the right frame bounds via service protocol.
@@ -746,7 +761,8 @@ class Shell final : public PlatformView::Delegate,
 
   // Service protocol handler
   //
-  // The returned SkSLs are base64 encoded. Decode before storing them to files.
+  // The returned SkSLs are base64 encoded. Decode before storing them to
+  // files.
   bool OnServiceProtocolGetSkSLs(
       const ServiceProtocol::Handler::ServiceProtocolMap& params,
       rapidjson::Document* response);
@@ -767,8 +783,8 @@ class Shell final : public PlatformView::Delegate,
 
   // Service protocol handler
   //
-  // Forces the FontCollection to reload the font manifest. Used to support hot
-  // reload for fonts.
+  // Forces the FontCollection to reload the font manifest. Used to support
+  // hot reload for fonts.
   bool OnServiceProtocolReloadAssetFonts(
       const ServiceProtocol::Handler::ServiceProtocolMap& params,
       rapidjson::Document* response);