From 9fc9cf4b5ac568ab20ea6941504a08795d089a8e Mon Sep 17 00:00:00 2001 From: Zachary Anderson Date: Wed, 8 Mar 2023 10:49:07 -0800 Subject: [PATCH] Revert "[engine] move asset mapping copy to background thread (#39918)" This reverts commit 7684aa0e9847db2dc5b1bd5670619a3c26f17280. --- fml/concurrent_message_loop.cc | 10 ---- fml/concurrent_message_loop.h | 2 - lib/ui/painting.dart | 7 +-- lib/ui/painting/immutable_buffer.cc | 64 ++++++------------------ shell/common/fixtures/shell_test.dart | 8 --- shell/common/shell_unittests.cc | 70 --------------------------- 6 files changed, 16 insertions(+), 145 deletions(-) diff --git a/fml/concurrent_message_loop.cc b/fml/concurrent_message_loop.cc index f553654a1195f..681b983fca3ed 100644 --- a/fml/concurrent_message_loop.cc +++ b/fml/concurrent_message_loop.cc @@ -170,14 +170,4 @@ void ConcurrentTaskRunner::PostTask(const fml::closure& task) { task(); } -bool ConcurrentMessageLoop::RunsTasksOnCurrentThread() { - std::scoped_lock lock(tasks_mutex_); - for (const auto& worker_thread_id : worker_thread_ids_) { - if (worker_thread_id == std::this_thread::get_id()) { - return true; - } - } - return false; -} - } // namespace fml diff --git a/fml/concurrent_message_loop.h b/fml/concurrent_message_loop.h index ab8534b6c946a..c043705935e57 100644 --- a/fml/concurrent_message_loop.h +++ b/fml/concurrent_message_loop.h @@ -34,8 +34,6 @@ class ConcurrentMessageLoop void PostTaskToAllWorkers(const fml::closure& task); - bool RunsTasksOnCurrentThread(); - private: friend ConcurrentTaskRunner; diff --git a/lib/ui/painting.dart b/lib/ui/painting.dart index ee2d173259216..30cc3d3ecc0ba 100644 --- a/lib/ui/painting.dart +++ b/lib/ui/painting.dart @@ -6294,12 +6294,7 @@ class ImmutableBuffer extends NativeFieldWrapperClass1 { final ImmutableBuffer instance = ImmutableBuffer._(0); return _futurize((_Callback callback) { return instance._initFromAsset(encodedKey, callback); - }).then((int length) { - if (length == -1) { - throw Exception('Asset not found'); - } - return instance.._length = length; - }); + }).then((int length) => instance.._length = length); } /// Create a buffer from the file with [path]. diff --git a/lib/ui/painting/immutable_buffer.cc b/lib/ui/painting/immutable_buffer.cc index 0595eb8dc93e3..0d86b07c24e4a 100644 --- a/lib/ui/painting/immutable_buffer.cc +++ b/lib/ui/painting/immutable_buffer.cc @@ -43,7 +43,7 @@ Dart_Handle ImmutableBuffer::init(Dart_Handle buffer_handle, return Dart_Null(); } -Dart_Handle ImmutableBuffer::initFromAsset(Dart_Handle raw_buffer_handle, +Dart_Handle ImmutableBuffer::initFromAsset(Dart_Handle buffer_handle, Dart_Handle asset_name_handle, Dart_Handle callback_handle) { UIDartState::ThrowIfUIOperationsProhibited(); @@ -62,55 +62,21 @@ Dart_Handle ImmutableBuffer::initFromAsset(Dart_Handle raw_buffer_handle, std::string asset_name = std::string{reinterpret_cast(chars), static_cast(asset_length)}; - auto* dart_state = UIDartState::Current(); - auto ui_task_runner = dart_state->GetTaskRunners().GetUITaskRunner(); - auto buffer_callback = - std::make_unique(dart_state, callback_handle); - auto buffer_handle = std::make_unique( - dart_state, raw_buffer_handle); - auto asset_manager = UIDartState::Current() - ->platform_configuration() - ->client() - ->GetAssetManager(); - - auto ui_task = fml::MakeCopyable( - [buffer_callback = std::move(buffer_callback), - buffer_handle = std::move(buffer_handle)](const sk_sp& sk_data, - size_t buffer_size) mutable { - auto dart_state = buffer_callback->dart_state().lock(); - if (!dart_state) { - return; - } - tonic::DartState::Scope scope(dart_state); - if (!sk_data) { - // -1 is used as a sentinel that the file could not be opened. - tonic::DartInvoke(buffer_callback->Get(), {tonic::ToDart(-1)}); - return; - } - auto buffer = fml::MakeRefCounted(sk_data); - buffer->AssociateWithDartWrapper(buffer_handle->Get()); - tonic::DartInvoke(buffer_callback->Get(), {tonic::ToDart(buffer_size)}); - }); - - dart_state->GetConcurrentTaskRunner()->PostTask( - [asset_name = std::move(asset_name), - asset_manager = std::move(asset_manager), - ui_task_runner = std::move(ui_task_runner), ui_task] { - std::unique_ptr mapping = - asset_manager->GetAsMapping(asset_name); + std::shared_ptr asset_manager = UIDartState::Current() + ->platform_configuration() + ->client() + ->GetAssetManager(); + std::unique_ptr data = asset_manager->GetAsMapping(asset_name); + if (data == nullptr) { + return tonic::ToDart("Asset not found"); + } - sk_sp sk_data; - size_t buffer_size = 0; - if (mapping != nullptr) { - buffer_size = mapping->GetSize(); - const void* bytes = static_cast(mapping->GetMapping()); - sk_data = MakeSkDataWithCopy(bytes, buffer_size); - } - ui_task_runner->PostTask( - [sk_data = std::move(sk_data), ui_task = ui_task, buffer_size]() { - ui_task(sk_data, buffer_size); - }); - }); + auto size = data->GetSize(); + const void* bytes = static_cast(data->GetMapping()); + auto sk_data = MakeSkDataWithCopy(bytes, size); + auto buffer = fml::MakeRefCounted(sk_data); + buffer->AssociateWithDartWrapper(buffer_handle); + tonic::DartInvoke(callback_handle, {tonic::ToDart(size)}); return Dart_Null(); } diff --git a/shell/common/fixtures/shell_test.dart b/shell/common/fixtures/shell_test.dart index b88289a093f74..605e3d951d230 100644 --- a/shell/common/fixtures/shell_test.dart +++ b/shell/common/fixtures/shell_test.dart @@ -470,11 +470,3 @@ Future testPluginUtilitiesCallbackHandle() async { } notifyNativeBool(true); } - -@pragma('vm:entry-point') -Future testThatAssetLoadingHappensOnWorkerThread() async { - try { - await ImmutableBuffer.fromAsset('DoesNotExist'); - } catch (err) { /* Do nothing */ } - notifyNative(); -} diff --git a/shell/common/shell_unittests.cc b/shell/common/shell_unittests.cc index 36c86463d8534..449822cd91c32 100644 --- a/shell/common/shell_unittests.cc +++ b/shell/common/shell_unittests.cc @@ -19,7 +19,6 @@ #include "flutter/flow/layers/layer_raster_cache_item.h" #include "flutter/flow/layers/platform_view_layer.h" #include "flutter/flow/layers/transform_layer.h" -#include "flutter/fml/backtrace.h" #include "flutter/fml/command_line.h" #include "flutter/fml/dart/dart_converter.h" #include "flutter/fml/make_copyable.h" @@ -195,42 +194,6 @@ class TestAssetResolver : public AssetResolver { AssetResolver::AssetResolverType type_; }; -class ThreadCheckingAssetResolver : public AssetResolver { - public: - explicit ThreadCheckingAssetResolver( - std::shared_ptr concurrent_loop) - : concurrent_loop_(std::move(concurrent_loop)) {} - - // |AssetResolver| - bool IsValid() const override { return true; } - - // |AssetResolver| - bool IsValidAfterAssetManagerChange() const override { return true; } - - // |AssetResolver| - AssetResolverType GetType() const { - return AssetResolverType::kApkAssetProvider; - } - - // |AssetResolver| - std::unique_ptr GetAsMapping( - const std::string& asset_name) const override { - if (asset_name == "FontManifest.json") { - // This file is loaded directly by the engine. - return nullptr; - } - mapping_requests.push_back(asset_name); - EXPECT_TRUE(concurrent_loop_->RunsTasksOnCurrentThread()) - << fml::BacktraceHere(); - return nullptr; - } - - mutable std::vector mapping_requests; - - private: - std::shared_ptr concurrent_loop_; -}; - static bool ValidateShell(Shell* shell) { if (!shell) { return false; @@ -3842,39 +3805,6 @@ TEST_F(ShellTest, SpawnWorksWithOnError) { ASSERT_FALSE(DartVMRef::IsInstanceRunning()); } -TEST_F(ShellTest, ImmutableBufferLoadsAssetOnBackgroundThread) { - Settings settings = CreateSettingsForFixture(); - auto task_runner = CreateNewThread(); - TaskRunners task_runners("test", task_runner, task_runner, task_runner, - task_runner); - std::unique_ptr shell = CreateShell(settings, task_runners); - - fml::CountDownLatch latch(1); - AddNativeCallback("NotifyNative", - CREATE_NATIVE_ENTRY([&](auto args) { latch.CountDown(); })); - - // Create the surface needed by rasterizer - PlatformViewNotifyCreated(shell.get()); - - auto configuration = RunConfiguration::InferFromSettings(settings); - configuration.SetEntrypoint("testThatAssetLoadingHappensOnWorkerThread"); - auto asset_manager = configuration.GetAssetManager(); - auto test_resolver = std::make_unique( - shell->GetDartVM()->GetConcurrentMessageLoop()); - auto leaked_resolver = test_resolver.get(); - asset_manager->PushBack(std::move(test_resolver)); - - RunEngine(shell.get(), std::move(configuration)); - PumpOneFrame(shell.get()); - - latch.Wait(); - - EXPECT_EQ(leaked_resolver->mapping_requests[0], "DoesNotExist"); - - PlatformViewNotifyDestroyed(shell.get()); - DestroyShell(std::move(shell), task_runners); -} - TEST_F(ShellTest, PictureToImageSync) { #if !SHELL_ENABLE_GL // This test uses the GL backend.