From ae7ee47f75c2fc01a7822bbf2519ff0a245d0810 Mon Sep 17 00:00:00 2001 From: Forrest Reiling Date: Tue, 30 Mar 2021 03:21:44 +0000 Subject: [PATCH 1/2] [fuchsia][shader warmup] Fixed SkpWarmupTest This test regressed due to https://github.com/flutter/engine/pull/23488 and this regression was silent due to https://github.com/flutter/flutter/issues/78277 Credit to @gnoliyil for actually putting together the fix. --- shell/platform/fuchsia/flutter/tests/engine_unittests.cc | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/shell/platform/fuchsia/flutter/tests/engine_unittests.cc b/shell/platform/fuchsia/flutter/tests/engine_unittests.cc index 95cf266384c3c..21d9492b66246 100644 --- a/shell/platform/fuchsia/flutter/tests/engine_unittests.cc +++ b/shell/platform/fuchsia/flutter/tests/engine_unittests.cc @@ -59,8 +59,9 @@ class EngineTest : public ::testing::Test { // otherwise we segfault creating the VulkanSurfaceProducer auto loop = fml::MessageLoopImpl::Create(); - fuchsia::ui::scenic::SessionPtr session_ptr; - scenic::Session session(std::move(session_ptr)); + context_ = sys::ComponentContext::CreateAndServeOutgoingDirectory(); + scenic_ = context_->svc()->Connect(); + scenic::Session session(scenic_.get()); surface_producer_ = std::make_unique(&session); Engine::WarmupSkps(&concurrent_task_runner_, &raster_task_runner_, @@ -71,6 +72,9 @@ class EngineTest : public ::testing::Test { MockTaskRunner concurrent_task_runner_; MockTaskRunner raster_task_runner_; std::unique_ptr surface_producer_; + + std::unique_ptr context_; + fuchsia::ui::scenic::ScenicPtr scenic_; }; TEST_F(EngineTest, SkpWarmup) { From f99dd7cd96012a0e19ace7b2e454e5f81c216f29 Mon Sep 17 00:00:00 2001 From: Forrest Reiling Date: Fri, 5 Mar 2021 22:00:06 +0000 Subject: [PATCH 2/2] [fuchsia][shader warmup] Avoid recursively iterating over assets directory when loading skp's due to high cost of openat() on pkgfs --- assets/asset_manager.cc | 5 +- assets/asset_manager.h | 4 +- assets/asset_resolver.h | 21 +++++- assets/directory_asset_bundle.cc | 31 +++++++- assets/directory_asset_bundle.h | 4 +- common/graphics/persistent_cache.cc | 2 +- fml/platform/posix/file_posix.cc | 2 + shell/common/shell_unittests.cc | 74 +++++++++++++++++-- .../fuchsia/flutter/tests/engine_unittests.cc | 4 +- 9 files changed, 129 insertions(+), 18 deletions(-) diff --git a/assets/asset_manager.cc b/assets/asset_manager.cc index b1c6e04b897f1..d52fc6eea7f2c 100644 --- a/assets/asset_manager.cc +++ b/assets/asset_manager.cc @@ -77,7 +77,8 @@ std::unique_ptr AssetManager::GetAsMapping( // |AssetResolver| std::vector> AssetManager::GetAsMappings( - const std::string& asset_pattern) const { + const std::string& asset_pattern, + const std::optional& subdir) const { std::vector> mappings; if (asset_pattern.size() == 0) { return mappings; @@ -85,7 +86,7 @@ std::vector> AssetManager::GetAsMappings( TRACE_EVENT1("flutter", "AssetManager::GetAsMappings", "pattern", asset_pattern.c_str()); for (const auto& resolver : resolvers_) { - auto resolver_mappings = resolver->GetAsMappings(asset_pattern); + auto resolver_mappings = resolver->GetAsMappings(asset_pattern, subdir); mappings.insert(mappings.end(), std::make_move_iterator(resolver_mappings.begin()), std::make_move_iterator(resolver_mappings.end())); diff --git a/assets/asset_manager.h b/assets/asset_manager.h index a116d6e906466..335bd84116365 100644 --- a/assets/asset_manager.h +++ b/assets/asset_manager.h @@ -9,6 +9,7 @@ #include #include +#include #include "flutter/assets/asset_resolver.h" #include "flutter/fml/macros.h" #include "flutter/fml/memory/ref_counted.h" @@ -70,7 +71,8 @@ class AssetManager final : public AssetResolver { // |AssetResolver| std::vector> GetAsMappings( - const std::string& asset_pattern) const override; + const std::string& asset_pattern, + const std::optional& subdir) const override; private: std::deque> resolvers_; diff --git a/assets/asset_resolver.h b/assets/asset_resolver.h index 0674e71121b23..54eb6812f8fff 100644 --- a/assets/asset_resolver.h +++ b/assets/asset_resolver.h @@ -8,6 +8,7 @@ #include #include +#include #include "flutter/fml/macros.h" #include "flutter/fml/mapping.h" @@ -59,10 +60,24 @@ class AssetResolver { [[nodiscard]] virtual std::unique_ptr GetAsMapping( const std::string& asset_name) const = 0; - // Same as GetAsMapping() but returns mappings for all files who's name - // matches |pattern|. Returns empty vector if no matching assets are found + //-------------------------------------------------------------------------- + /// @brief Same as GetAsMapping() but returns mappings for all files + /// who's name matches a given pattern. Returns empty vector + /// if no matching assets are found. + /// + /// @param[in] asset_pattern The pattern to match file names against. + /// + /// @param[in] subdir Optional subdirectory in which to search for files. + /// If supplied this function does a flat search within the + /// subdirectory instead of a recursive search through the entire + /// assets directory. + /// + /// @return Returns a vector of mappings of files which match the search + /// parameters. + /// [[nodiscard]] virtual std::vector> - GetAsMappings(const std::string& asset_pattern) const { + GetAsMappings(const std::string& asset_pattern, + const std::optional& subdir) const { return {}; }; diff --git a/assets/directory_asset_bundle.cc b/assets/directory_asset_bundle.cc index 45ab571823902..7896756614593 100644 --- a/assets/directory_asset_bundle.cc +++ b/assets/directory_asset_bundle.cc @@ -10,6 +10,7 @@ #include "flutter/fml/eintr_wrapper.h" #include "flutter/fml/file.h" #include "flutter/fml/mapping.h" +#include "flutter/fml/trace_event.h" namespace flutter { @@ -60,7 +61,8 @@ std::unique_ptr DirectoryAssetBundle::GetAsMapping( } std::vector> DirectoryAssetBundle::GetAsMappings( - const std::string& asset_pattern) const { + const std::string& asset_pattern, + const std::optional& subdir) const { std::vector> mappings; if (!is_valid_) { FML_DLOG(WARNING) << "Asset bundle was not valid."; @@ -70,9 +72,19 @@ std::vector> DirectoryAssetBundle::GetAsMappings( std::regex asset_regex(asset_pattern); fml::FileVisitor visitor = [&](const fml::UniqueFD& directory, const std::string& filename) { + TRACE_EVENT0("flutter", "DirectoryAssetBundle::GetAsMappings FileVisitor"); + if (std::regex_match(filename, asset_regex)) { - auto mapping = std::make_unique(fml::OpenFile( - directory, filename.c_str(), false, fml::FilePermission::kRead)); + TRACE_EVENT0("flutter", "Matched File"); + + fml::UniqueFD fd = fml::OpenFile(directory, filename.c_str(), false, + fml::FilePermission::kRead); + + if (fml::IsDirectory(fd)) { + return true; + } + + auto mapping = std::make_unique(fd); if (mapping && mapping->IsValid()) { mappings.push_back(std::move(mapping)); @@ -82,7 +94,18 @@ std::vector> DirectoryAssetBundle::GetAsMappings( } return true; }; - fml::VisitFilesRecursively(descriptor_, visitor); + if (!subdir) { + fml::VisitFilesRecursively(descriptor_, visitor); + } else { + fml::UniqueFD subdir_fd = + fml::OpenFileReadOnly(descriptor_, subdir.value().c_str()); + if (!fml::IsDirectory(subdir_fd)) { + FML_LOG(ERROR) << "Subdirectory path " << subdir.value() + << " is not a directory"; + return mappings; + } + fml::VisitFiles(subdir_fd, visitor); + } return mappings; } diff --git a/assets/directory_asset_bundle.h b/assets/directory_asset_bundle.h index b004201e0ad96..3621300f39954 100644 --- a/assets/directory_asset_bundle.h +++ b/assets/directory_asset_bundle.h @@ -5,6 +5,7 @@ #ifndef FLUTTER_ASSETS_DIRECTORY_ASSET_BUNDLE_H_ #define FLUTTER_ASSETS_DIRECTORY_ASSET_BUNDLE_H_ +#include #include "flutter/assets/asset_resolver.h" #include "flutter/fml/macros.h" #include "flutter/fml/memory/ref_counted.h" @@ -39,7 +40,8 @@ class DirectoryAssetBundle : public AssetResolver { // |AssetResolver| std::vector> GetAsMappings( - const std::string& asset_pattern) const override; + const std::string& asset_pattern, + const std::optional& subdir) const override; FML_DISALLOW_COPY_AND_ASSIGN(DirectoryAssetBundle); }; diff --git a/common/graphics/persistent_cache.cc b/common/graphics/persistent_cache.cc index efbf676cc2ced..360b32e07d88f 100644 --- a/common/graphics/persistent_cache.cc +++ b/common/graphics/persistent_cache.cc @@ -410,7 +410,7 @@ PersistentCache::GetSkpsFromAssetManager() const { << "PersistentCache::GetSkpsFromAssetManager: Asset manager not set!"; return std::vector>(); } - return asset_manager_->GetAsMappings(".*\\.skp$"); + return asset_manager_->GetAsMappings(".*\\.skp$", "shaders"); } } // namespace flutter diff --git a/fml/platform/posix/file_posix.cc b/fml/platform/posix/file_posix.cc index f6a90942a9391..018be839c1701 100644 --- a/fml/platform/posix/file_posix.cc +++ b/fml/platform/posix/file_posix.cc @@ -16,6 +16,7 @@ #include "flutter/fml/eintr_wrapper.h" #include "flutter/fml/logging.h" #include "flutter/fml/mapping.h" +#include "flutter/fml/trace_event.h" #include "flutter/fml/unique_fd.h" namespace fml { @@ -72,6 +73,7 @@ fml::UniqueFD OpenFile(const fml::UniqueFD& base_directory, const char* path, bool create_if_necessary, FilePermission permission) { + TRACE_EVENT0("flutter", "fml::OpenFile"); if (path == nullptr) { return {}; } diff --git a/shell/common/shell_unittests.cc b/shell/common/shell_unittests.cc index b9bec86095648..0daddce767641 100644 --- a/shell/common/shell_unittests.cc +++ b/shell/common/shell_unittests.cc @@ -139,7 +139,8 @@ class TestAssetResolver : public AssetResolver { } std::vector> GetAsMappings( - const std::string& asset_pattern) const override { + const std::string& asset_pattern, + const std::optional& subdir) const override { return {}; }; @@ -2436,16 +2437,78 @@ TEST_F(ShellTest, AssetManagerMulti) { asset_manager.PushBack( std::make_unique(std::move(asset_dir_fd), false)); - auto mappings = asset_manager.GetAsMappings("(.*)"); - ASSERT_TRUE(mappings.size() == 4); + auto mappings = asset_manager.GetAsMappings("(.*)", std::nullopt); + EXPECT_EQ(mappings.size(), 4u); std::vector expected_results = { "good0", "good1", }; - mappings = asset_manager.GetAsMappings("(.*)good(.*)"); - ASSERT_TRUE(mappings.size() == expected_results.size()); + mappings = asset_manager.GetAsMappings("(.*)good(.*)", std::nullopt); + ASSERT_EQ(mappings.size(), expected_results.size()); + + for (auto& mapping : mappings) { + std::string result(reinterpret_cast(mapping->GetMapping()), + mapping->GetSize()); + EXPECT_NE( + std::find(expected_results.begin(), expected_results.end(), result), + expected_results.end()); + } +} + +#if defined(OS_FUCHSIA) +TEST_F(ShellTest, AssetManagerMultiSubdir) { + std::string subdir_path = "subdir"; + + fml::ScopedTemporaryDirectory asset_dir; + fml::UniqueFD asset_dir_fd = fml::OpenDirectory( + asset_dir.path().c_str(), false, fml::FilePermission::kRead); + fml::UniqueFD subdir_fd = + fml::OpenDirectory((asset_dir.path() + "/" + subdir_path).c_str(), true, + fml::FilePermission::kReadWrite); + + std::vector filenames = { + "bad0", + "notgood", // this is to make sure the pattern (.*)good(.*) only matches + // things in the subdirectory + }; + + std::vector subdir_filenames = { + "good0", + "good1", + "bad1", + }; + + for (auto filename : filenames) { + bool success = fml::WriteAtomically(asset_dir_fd, filename.c_str(), + fml::DataMapping(filename)); + ASSERT_TRUE(success); + } + + for (auto filename : subdir_filenames) { + bool success = fml::WriteAtomically(subdir_fd, filename.c_str(), + fml::DataMapping(filename)); + ASSERT_TRUE(success); + } + + AssetManager asset_manager; + asset_manager.PushBack( + std::make_unique(std::move(asset_dir_fd), false)); + + auto mappings = asset_manager.GetAsMappings("(.*)", std::nullopt); + EXPECT_EQ(mappings.size(), 5u); + + mappings = asset_manager.GetAsMappings("(.*)", subdir_path); + EXPECT_EQ(mappings.size(), 3u); + + std::vector expected_results = { + "good0", + "good1", + }; + + mappings = asset_manager.GetAsMappings("(.*)good(.*)", subdir_path); + ASSERT_EQ(mappings.size(), expected_results.size()); for (auto& mapping : mappings) { std::string result(reinterpret_cast(mapping->GetMapping()), @@ -2455,6 +2518,7 @@ TEST_F(ShellTest, AssetManagerMulti) { expected_results.end()); } } +#endif // OS_FUCHSIA TEST_F(ShellTest, Spawn) { auto settings = CreateSettingsForFixture(); diff --git a/shell/platform/fuchsia/flutter/tests/engine_unittests.cc b/shell/platform/fuchsia/flutter/tests/engine_unittests.cc index 21d9492b66246..78c0235180a1f 100644 --- a/shell/platform/fuchsia/flutter/tests/engine_unittests.cc +++ b/shell/platform/fuchsia/flutter/tests/engine_unittests.cc @@ -102,8 +102,10 @@ TEST_F(EngineTest, SkpWarmup) { fml::ScopedTemporaryDirectory asset_dir; fml::UniqueFD asset_dir_fd = fml::OpenDirectory( asset_dir.path().c_str(), false, fml::FilePermission::kRead); + fml::UniqueFD subdir_fd = fml::OpenDirectory(asset_dir_fd, "shaders", true, + fml::FilePermission::kReadWrite); - bool success = fml::WriteAtomically(asset_dir_fd, "test.skp", mapping); + bool success = fml::WriteAtomically(subdir_fd, "test.skp", mapping); ASSERT_TRUE(success); auto asset_manager = std::make_shared();