[fuchsia][shader warmup] Avoid recursively iterating over assets dire…

…ctory when loading skp's due to high cost of openat() on pkgfs (flutter#25006)

* [fuchsia][shader warmup] Fixed SkpWarmupTest

This test regressed due to flutter#23488
and this regression was silent due to flutter/flutter#78277

Credit to @gnoliyil for actually putting together the fix.

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

@@ -77,15 +77,16 @@ std::unique_ptr<fml::Mapping> AssetManager::GetAsMapping(
 
 // |AssetResolver|
 std::vector<std::unique_ptr<fml::Mapping>> AssetManager::GetAsMappings(
-    const std::string& asset_pattern) const {
+    const std::string& asset_pattern,
+    const std::optional<std::string>& subdir) const {
   std::vector<std::unique_ptr<fml::Mapping>> mappings;
   if (asset_pattern.size() == 0) {
     return mappings;
   }
   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()));

assets/asset_manager.h

@@ -9,6 +9,7 @@
 #include <memory>
 #include <string>
 
+#include <optional>
 #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<std::unique_ptr<fml::Mapping>> GetAsMappings(
-      const std::string& asset_pattern) const override;
+      const std::string& asset_pattern,
+      const std::optional<std::string>& subdir) const override;
 
  private:
   std::deque<std::unique_ptr<AssetResolver>> resolvers_;

assets/asset_resolver.h

@@ -8,6 +8,7 @@
 #include <string>
 #include <vector>
 
+#include <optional>
 #include "flutter/fml/macros.h"
 #include "flutter/fml/mapping.h"
 
@@ -59,10 +60,24 @@ class AssetResolver {
   [[nodiscard]] virtual std::unique_ptr<fml::Mapping> 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<std::unique_ptr<fml::Mapping>>
-  GetAsMappings(const std::string& asset_pattern) const {
+  GetAsMappings(const std::string& asset_pattern,
+                const std::optional<std::string>& subdir) const {
     return {};
   };
 

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<fml::Mapping> DirectoryAssetBundle::GetAsMapping(
 }
 
 std::vector<std::unique_ptr<fml::Mapping>> DirectoryAssetBundle::GetAsMappings(
-    const std::string& asset_pattern) const {
+    const std::string& asset_pattern,
+    const std::optional<std::string>& subdir) const {
   std::vector<std::unique_ptr<fml::Mapping>> mappings;
   if (!is_valid_) {
     FML_DLOG(WARNING) << "Asset bundle was not valid.";
@@ -70,9 +72,19 @@ std::vector<std::unique_ptr<fml::Mapping>> 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::FileMapping>(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<fml::FileMapping>(fd);
 
       if (mapping && mapping->IsValid()) {
         mappings.push_back(std::move(mapping));
@@ -82,7 +94,18 @@ std::vector<std::unique_ptr<fml::Mapping>> 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;
 }

assets/directory_asset_bundle.h

@@ -5,6 +5,7 @@
 #ifndef FLUTTER_ASSETS_DIRECTORY_ASSET_BUNDLE_H_
 #define FLUTTER_ASSETS_DIRECTORY_ASSET_BUNDLE_H_
 
+#include <optional>
 #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<std::unique_ptr<fml::Mapping>> GetAsMappings(
-      const std::string& asset_pattern) const override;
+      const std::string& asset_pattern,
+      const std::optional<std::string>& subdir) const override;
 
   FML_DISALLOW_COPY_AND_ASSIGN(DirectoryAssetBundle);
 };

common/graphics/persistent_cache.cc

@@ -410,7 +410,7 @@ PersistentCache::GetSkpsFromAssetManager() const {
         << "PersistentCache::GetSkpsFromAssetManager: Asset manager not set!";
     return std::vector<std::unique_ptr<fml::Mapping>>();
   }
-  return asset_manager_->GetAsMappings(".*\\.skp$");
+  return asset_manager_->GetAsMappings(".*\\.skp$", "shaders");
 }
 
 }  // namespace flutter

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 {};
   }

shell/common/shell_unittests.cc

@@ -139,7 +139,8 @@ class TestAssetResolver : public AssetResolver {
   }
 
   std::vector<std::unique_ptr<fml::Mapping>> GetAsMappings(
-      const std::string& asset_pattern) const override {
+      const std::string& asset_pattern,
+      const std::optional<std::string>& subdir) const override {
     return {};
   };
 
@@ -2436,16 +2437,78 @@ TEST_F(ShellTest, AssetManagerMulti) {
   asset_manager.PushBack(
       std::make_unique<DirectoryAssetBundle>(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<std::string> 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<const char*>(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<std::string> filenames = {
+      "bad0",
+      "notgood",  // this is to make sure the pattern (.*)good(.*) only matches
+                  // things in the subdirectory
+  };
+
+  std::vector<std::string> 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<DirectoryAssetBundle>(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<std::string> 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<const char*>(mapping->GetMapping()),
@@ -2455,6 +2518,7 @@ TEST_F(ShellTest, AssetManagerMulti) {
         expected_results.end());
   }
 }
+#endif  // OS_FUCHSIA
 
 TEST_F(ShellTest, Spawn) {
   auto settings = CreateSettingsForFixture();

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<fuchsia::ui::scenic::Scenic>();
+    scenic::Session session(scenic_.get());
     surface_producer_ = std::make_unique<VulkanSurfaceProducer>(&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<VulkanSurfaceProducer> surface_producer_;
+
+  std::unique_ptr<sys::ComponentContext> context_;
+  fuchsia::ui::scenic::ScenicPtr scenic_;
 };
 
 TEST_F(EngineTest, SkpWarmup) {
@@ -98,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<AssetManager>();