From 6584b2c308cf34d30342901153d60f86152b2acb Mon Sep 17 00:00:00 2001 From: Tong Mu Date: Wed, 18 Oct 2023 14:09:01 -0700 Subject: [PATCH] Reland 2 (part 1): Enforce the rule of calling `FlutterView.Render` (flutter/engine#47062) This PR relands part of https://github.com/flutter/engine/pull/45300, which was reverted in https://github.com/flutter/engine/pull/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 https://github.com/flutter/flutter/issues/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]. [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 --- engine/src/flutter/shell/common/shell.cc | 31 +++++++++++-------- engine/src/flutter/shell/common/shell.h | 38 +++++++++++++++++------- 2 files changed, 45 insertions(+), 24 deletions(-) diff --git a/engine/src/flutter/shell/common/shell.cc b/engine/src/flutter/shell/common/shell.cc index a7f7c9ae4c..a6ba487b58 100644 --- a/engine/src/flutter/shell/common/shell.cc +++ b/engine/src/flutter/shell/common/shell.cc @@ -144,6 +144,23 @@ void PerformInitializationTasks(Settings& settings) { } // namespace +std::pair> +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); + + // 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::Create( const PlatformData& platform_data, const TaskRunners& task_runners, @@ -156,19 +173,7 @@ std::unique_ptr Shell::Create( TRACE_EVENT0("flutter", "Shell::Create"); - // 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(); - } + auto [vm, isolate_snapshot] = InferVmInitDataFromSettings(settings); auto resource_cache_limit_calculator = std::make_shared( settings.resource_cache_max_bytes_threshold); diff --git a/engine/src/flutter/shell/common/shell.h b/engine/src/flutter/shell/common/shell.h index 039eb653a0..9eb5bd1e9b 100644 --- a/engine/src/flutter/shell/common/shell.h +++ b/engine/src/flutter/shell/common/shell.h @@ -438,15 +438,29 @@ class Shell final : public PlatformView::Delegate, const std::shared_ptr 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> + InferVmInitDataFromSettings(Settings& settings); + private: using ServiceProtocolHandler = std::function; /// 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 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 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 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 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);