From 6d2e85e40df7d2ac7a8ed1c7264fe8211b17a64e Mon Sep 17 00:00:00 2001 From: Yuqian Li Date: Thu, 12 Sep 2019 16:52:13 -0700 Subject: [PATCH 1/6] SkSL precompile --- common/settings.cc | 1 + common/settings.h | 1 + fml/file.h | 2 + fml/platform/posix/file_posix.cc | 17 ++++ shell/common/persistent_cache.cc | 93 +++++++++++++++---- shell/common/persistent_cache.h | 32 ++++++- shell/common/shell.cc | 2 + shell/common/switches.cc | 3 + shell/common/switches.h | 6 ++ shell/gpu/gpu_surface_gl.cc | 19 +++- .../flutter/app/FlutterActivityDelegate.java | 3 + .../embedding/engine/FlutterShellArgs.java | 7 +- shell/platform/embedder/embedder.h | 3 +- 13 files changed, 163 insertions(+), 26 deletions(-) diff --git a/common/settings.cc b/common/settings.cc index b4b58cb6d7714..b1ef2161fa561 100644 --- a/common/settings.cc +++ b/common/settings.cc @@ -40,6 +40,7 @@ std::string Settings::ToString() const { stream << "trace_systrace: " << trace_systrace << std::endl; stream << "dump_skp_on_shader_compilation: " << dump_skp_on_shader_compilation << std::endl; + stream << "cache_sksl: " << cache_sksl << std::endl; stream << "endless_trace_buffer: " << endless_trace_buffer << std::endl; stream << "enable_dart_profiling: " << enable_dart_profiling << std::endl; stream << "disable_dart_asserts: " << disable_dart_asserts << std::endl; diff --git a/common/settings.h b/common/settings.h index d39c2ea6ea3da..2ebcbf0cd862a 100644 --- a/common/settings.h +++ b/common/settings.h @@ -94,6 +94,7 @@ struct Settings { bool trace_startup = false; bool trace_systrace = false; bool dump_skp_on_shader_compilation = false; + bool cache_sksl = false; bool endless_trace_buffer = false; bool enable_dart_profiling = false; bool disable_dart_asserts = false; diff --git a/fml/file.h b/fml/file.h index 068ddc84c999d..4df17ce2d73e9 100644 --- a/fml/file.h +++ b/fml/file.h @@ -73,6 +73,8 @@ bool WriteAtomically(const fml::UniqueFD& base_directory, const char* file_name, const Mapping& mapping); +std::vector ListFiles(const fml::UniqueFD& directory); + class ScopedTemporaryDirectory { public: ScopedTemporaryDirectory(); diff --git a/fml/platform/posix/file_posix.cc b/fml/platform/posix/file_posix.cc index d30a9c2f1d2df..28d0ba919a8f0 100644 --- a/fml/platform/posix/file_posix.cc +++ b/fml/platform/posix/file_posix.cc @@ -4,6 +4,7 @@ #include "flutter/fml/file.h" +#include #include #include #include @@ -210,4 +211,20 @@ bool WriteAtomically(const fml::UniqueFD& base_directory, base_directory.get(), file_name) == 0; } +std::vector ListFiles(const fml::UniqueFD& directory) { + std::vector files; + DIR* dir = ::fdopendir(directory.get()); + if (dir == nullptr) { + FML_LOG(ERROR) << "Can't open the directory."; + return files; + } + while (dirent* ent = readdir(dir)) { + std::string name = ent->d_name; + if (name != "." && name != "..") { + files.push_back(std::move(name)); + } + } + return files; +} + } // namespace fml diff --git a/shell/common/persistent_cache.cc b/shell/common/persistent_cache.cc index bc19976b38263..84ddb75bcfd51 100644 --- a/shell/common/persistent_cache.cc +++ b/shell/common/persistent_cache.cc @@ -10,6 +10,7 @@ #include "flutter/fml/base32.h" #include "flutter/fml/file.h" +#include "flutter/fml/logging.h" #include "flutter/fml/make_copyable.h" #include "flutter/fml/mapping.h" #include "flutter/fml/paths.h" @@ -39,6 +40,18 @@ static std::string SkKeyToFilePath(const SkData& data) { bool PersistentCache::gIsReadOnly = false; +std::atomic PersistentCache::cache_sksl_ = false; +std::atomic PersistentCache::strategy_set_ = false; + +void PersistentCache::SetCacheSkSL(bool value) { + if (strategy_set_) { + FML_LOG(ERROR) << "Cache SkSL can only be set before the " + "GrContextOptions::fShaderCacheStrategy is set."; + return; + } + cache_sksl_ = value; +} + PersistentCache* PersistentCache::GetCacheForProcess() { static std::unique_ptr gPersistentCache; static std::once_flag once = {}; @@ -52,9 +65,10 @@ void PersistentCache::SetCacheDirectoryPath(std::string path) { } namespace { -std::shared_ptr MakeCacheDirectory( +static std::shared_ptr MakeCacheDirectory( const std::string& global_cache_base_path, - bool read_only) { + bool read_only, + bool cache_sksl) { fml::UniqueFD cache_base_dir; if (global_cache_base_path.length()) { cache_base_dir = fml::OpenDirectory(global_cache_base_path.c_str(), false, @@ -64,20 +78,52 @@ std::shared_ptr MakeCacheDirectory( } if (cache_base_dir.is_valid()) { - return std::make_shared(CreateDirectory( - cache_base_dir, - {"flutter_engine", GetFlutterEngineVersion(), "skia", GetSkiaVersion()}, - read_only ? fml::FilePermission::kRead - : fml::FilePermission::kReadWrite)); + std::vector components = { + "flutter_engine", GetFlutterEngineVersion(), "skia", GetSkiaVersion()}; + if (cache_sksl) { + components.push_back("sksl"); + } + return std::make_shared( + CreateDirectory(cache_base_dir, components, + read_only ? fml::FilePermission::kRead + : fml::FilePermission::kReadWrite)); } else { return std::make_shared(); } } } // namespace +std::vector PersistentCache::LoadSkSLs() { + TRACE_EVENT0("flutter", "PersistentCache::LoadSkSLs"); + std::vector result; + if (!IsValid()) { + return result; + } + const char* path = cache_sksl_ ? "." : "sksl"; + fml::UniqueFD dir = fml::OpenDirectory(*cache_directory_, path, false, + fml::FilePermission::kRead); + std::vector names = fml::ListFiles(dir); + for (const std::string& name : names) { + std::pair decode_result = fml::Base32Decode(name); + if (!decode_result.first) { + FML_LOG(ERROR) << "Base32 can't decode: " << name; + continue; + } + const std::string& data_string = decode_result.second; + sk_sp key = + SkData::MakeWithCopy(data_string.data(), data_string.length()); + sk_sp data = LoadFile(dir, name); + if (data != nullptr) { + result.push_back({key, data}); + } + } + return result; +} + PersistentCache::PersistentCache(bool read_only) : is_read_only_(read_only), - cache_directory_(MakeCacheDirectory(cache_base_path_, read_only)) { + cache_directory_( + MakeCacheDirectory(cache_base_path_, read_only, cache_sksl_)) { if (!IsValid()) { FML_LOG(WARNING) << "Could not acquire the persistent cache directory. " "Caching of GPU resources on disk is disabled."; @@ -90,6 +136,20 @@ bool PersistentCache::IsValid() const { return cache_directory_ && cache_directory_->is_valid(); } +sk_sp PersistentCache::LoadFile(const fml::UniqueFD& dir, + const std::string& file_name) { + auto file = + fml::OpenFile(dir, file_name.c_str(), false, fml::FilePermission::kRead); + if (!file.is_valid()) { + return nullptr; + } + auto mapping = std::make_unique(file); + if (mapping->GetSize() == 0) { + return nullptr; + } + return SkData::MakeWithCopy(mapping->GetMapping(), mapping->GetSize()); +} + // |GrContextOptions::PersistentCache| sk_sp PersistentCache::load(const SkData& key) { TRACE_EVENT0("flutter", "PersistentCacheLoad"); @@ -100,18 +160,13 @@ sk_sp PersistentCache::load(const SkData& key) { if (file_name.size() == 0) { return nullptr; } - auto file = fml::OpenFile(*cache_directory_, file_name.c_str(), false, - fml::FilePermission::kRead); - if (!file.is_valid()) { - return nullptr; - } - auto mapping = std::make_unique(file); - if (mapping->GetSize() == 0) { - return nullptr; + auto result = PersistentCache::LoadFile(*cache_directory_, file_name); + if (result != nullptr) { + TRACE_EVENT0("flutter", "PersistentCacheLoadHit"); + } else { + FML_LOG(INFO) << "PersistentCache::load failed: " << file_name; } - - TRACE_EVENT0("flutter", "PersistentCacheLoadHit"); - return SkData::MakeWithCopy(mapping->GetMapping(), mapping->GetSize()); + return result; } static void PersistentCacheStore(fml::RefPtr worker, diff --git a/shell/common/persistent_cache.h b/shell/common/persistent_cache.h index 595593eba7227..fcc7783922f1d 100644 --- a/shell/common/persistent_cache.h +++ b/shell/common/persistent_cache.h @@ -49,9 +49,35 @@ class PersistentCache : public GrContextOptions::PersistentCache { bool IsDumpingSkp() const { return is_dumping_skp_; } void SetIsDumpingSkp(bool value) { is_dumping_skp_ = value; } + // Return a list of file names inside the sksl cache directory. The name is + // the Base32 encoding of the SkSL key (of type SkData). + std::vector ListSkSL() const; + + // |GrContextOptions::PersistentCache| + sk_sp load(const SkData& key) override; + + using SkSLCache = std::pair, sk_sp>; + + /// Load all the SkSL shader caches in the right directory. + std::vector LoadSkSLs(); + + static bool cache_sksl() { return cache_sksl_; } + static void SetCacheSkSL(bool value); + static void MarkStrategySet() { strategy_set_ = true; } + private: static std::string cache_base_path_; + // Mutable static switch that can be set before GetCacheForProcess is called + // and GrContextOptions.fShaderCacheStrategy is set. If true, it means that + // we'll set `GrContextOptions::fShaderCacheStrategy` to `kSkSL`, and all the + // persistent cache should be stored and loaded from the "sksl" directory. + static std::atomic cache_sksl_; + + // Guard flag to make sure that cache_sksl_ is not modified after + // cache_strategy_set_ becomes true. + static std::atomic strategy_set_; + const bool is_read_only_; const std::shared_ptr cache_directory_; mutable std::mutex worker_task_runners_mutex_; @@ -61,13 +87,13 @@ class PersistentCache : public GrContextOptions::PersistentCache { bool stored_new_shaders_ = false; bool is_dumping_skp_ = false; + static sk_sp LoadFile(const fml::UniqueFD& dir, + const std::string& filen_ame); + bool IsValid() const; PersistentCache(bool read_only = false); - // |GrContextOptions::PersistentCache| - sk_sp load(const SkData& key) override; - // |GrContextOptions::PersistentCache| void store(const SkData& key, const SkData& data) override; diff --git a/shell/common/shell.cc b/shell/common/shell.cc index 9c97908c399f6..a0e8a5be55b3a 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -199,6 +199,8 @@ static void PerformInitializationTasks(const Settings& settings) { FML_DLOG(WARNING) << "Skipping ICU initialization in the shell."; } } + + PersistentCache::SetCacheSkSL(settings.cache_sksl); }); } diff --git a/shell/common/switches.cc b/shell/common/switches.cc index ec275b36a475c..ad3b322acad4f 100644 --- a/shell/common/switches.cc +++ b/shell/common/switches.cc @@ -355,6 +355,9 @@ Settings SettingsFromCommandLine(const fml::CommandLine& command_line) { settings.dump_skp_on_shader_compilation = command_line.HasOption(FlagForSwitch(Switch::DumpSkpOnShaderCompilation)); + settings.cache_sksl = + command_line.HasOption(FlagForSwitch(Switch::CacheSkSL)); + return settings; } diff --git a/shell/common/switches.h b/shell/common/switches.h index 60a9392eeb4a2..29e4c81ea455c 100644 --- a/shell/common/switches.h +++ b/shell/common/switches.h @@ -130,6 +130,12 @@ DEF_SWITCH(DumpSkpOnShaderCompilation, "Automatically dump the skp that triggers new shader compilations. " "This is useful for writing custom ShaderWarmUp to reduce jank. " "By default, this is not enabled to reduce the overhead. ") +DEF_SWITCH(CacheSkSL, + "cache-sksl", + "Only cache the shader in SkSL instead of binary or GLSL. This " + "should only be used during development phases. The generated SkSLs " + "can later be used in the release build for shader precompilation " + "at launch in order to eliminate the shader-compile jank.") DEF_SWITCH( TraceSystrace, "trace-systrace", diff --git a/shell/gpu/gpu_surface_gl.cc b/shell/gpu/gpu_surface_gl.cc index 3d02bb59f19c0..0aeeb3d78e65e 100644 --- a/shell/gpu/gpu_surface_gl.cc +++ b/shell/gpu/gpu_surface_gl.cc @@ -4,6 +4,7 @@ #include "gpu_surface_gl.h" +#include "flutter/fml/base32.h" #include "flutter/fml/logging.h" #include "flutter/fml/size.h" #include "flutter/fml/trace_event.h" @@ -46,6 +47,11 @@ GPUSurfaceGL::GPUSurfaceGL(GPUSurfaceGLDelegate* delegate, GrContextOptions options; + if (PersistentCache::cache_sksl()) { + FML_LOG(INFO) << "Cache SkSL"; + options.fShaderCacheStrategy = GrContextOptions::ShaderCacheStrategy::kSkSL; + PersistentCache::MarkStrategySet(); + } options.fPersistentCache = PersistentCache::GetCacheForProcess(); options.fAvoidStencilBuffers = true; @@ -69,11 +75,20 @@ GPUSurfaceGL::GPUSurfaceGL(GPUSurfaceGLDelegate* delegate, context_->setResourceCacheLimits(kGrCacheMaxCount, kGrCacheMaxByteSize); - delegate_->GLContextClearCurrent(); - context_owner_ = true; valid_ = true; + + std::vector caches = + PersistentCache::GetCacheForProcess()->LoadSkSLs(); + int compiled_count = 0; + for (const auto& cache : caches) { + compiled_count += context_->precompileShader(*cache.first, *cache.second); + } + FML_LOG(INFO) << "Found " << caches.size() << " SkSL shaders; precompiled " + << compiled_count; + + delegate_->GLContextClearCurrent(); } GPUSurfaceGL::GPUSurfaceGL(sk_sp gr_context, diff --git a/shell/platform/android/io/flutter/app/FlutterActivityDelegate.java b/shell/platform/android/io/flutter/app/FlutterActivityDelegate.java index 95d007a809895..aa57d50905ebe 100644 --- a/shell/platform/android/io/flutter/app/FlutterActivityDelegate.java +++ b/shell/platform/android/io/flutter/app/FlutterActivityDelegate.java @@ -309,6 +309,9 @@ private static String[] getArgsFromIntent(Intent intent) { if (intent.getBooleanExtra("dump-skp-on-shader-compilation", false)) { args.add("--dump-skp-on-shader-compilation"); } + if (intent.getBooleanExtra("cache-sksl", false)) { + args.add("--cache-sksl"); + } if (intent.getBooleanExtra("verbose-logging", false)) { args.add("--verbose-logging"); } diff --git a/shell/platform/android/io/flutter/embedding/engine/FlutterShellArgs.java b/shell/platform/android/io/flutter/embedding/engine/FlutterShellArgs.java index ca37866906c37..544411b275742 100644 --- a/shell/platform/android/io/flutter/embedding/engine/FlutterShellArgs.java +++ b/shell/platform/android/io/flutter/embedding/engine/FlutterShellArgs.java @@ -41,6 +41,8 @@ public class FlutterShellArgs { public static final String ARG_TRACE_SKIA = "--trace-skia"; public static final String ARG_KEY_DUMP_SHADER_SKP_ON_SHADER_COMPILATION = "dump-skp-on-shader-compilation"; public static final String ARG_DUMP_SHADER_SKP_ON_SHADER_COMPILATION = "--dump-skp-on-shader-compilation"; + public static final String ARG_KEY_CACHE_SKSL = "cache-sksl"; + public static final String ARG_CACHE_SKSL = "--cache-sksl"; public static final String ARG_KEY_VERBOSE_LOGGING = "verbose-logging"; public static final String ARG_VERBOSE_LOGGING = "--verbose-logging"; public static final String ARG_KEY_OBSERVATORY_PORT = "observatory-port"; @@ -83,7 +85,10 @@ public static FlutterShellArgs fromIntent(@NonNull Intent intent) { args.add(ARG_TRACE_SKIA); } if (intent.getBooleanExtra(ARG_KEY_DUMP_SHADER_SKP_ON_SHADER_COMPILATION, false)) { - args.add(ARG_KEY_DUMP_SHADER_SKP_ON_SHADER_COMPILATION); + args.add(ARG_DUMP_SHADER_SKP_ON_SHADER_COMPILATION); + } + if (intent.getBooleanExtra(ARG_KEY_CACHE_SKSL, false)) { + args.add(ARG_CACHE_SKSL); } if (intent.getBooleanExtra(ARG_KEY_VERBOSE_LOGGING, false)) { args.add(ARG_VERBOSE_LOGGING); diff --git a/shell/platform/embedder/embedder.h b/shell/platform/embedder/embedder.h index 7490198b24aec..f721e209d5c32 100644 --- a/shell/platform/embedder/embedder.h +++ b/shell/platform/embedder/embedder.h @@ -875,7 +875,8 @@ typedef struct { // which is used in `flutter::Settings` as `temp_directory_path`. const char* persistent_cache_path; - /// If true, we'll only read the existing cache, but not write new ones. + /// If true, the engine would only read the existing cache, but not write new + /// ones. bool is_persistent_cache_read_only; /// A callback that gets invoked by the engine when it attempts to wait for a From d7fd345c999443ae8d357cf1bd544b9d6a0496b6 Mon Sep 17 00:00:00 2001 From: Yuqian Li Date: Wed, 25 Sep 2019 17:17:21 -0700 Subject: [PATCH 2/6] Change ListFiles to VisitFiles and add a unit test --- fml/file.h | 8 ++++++- fml/file_unittest.cc | 38 ++++++++++++++++++++++++++++++++ fml/platform/posix/file_posix.cc | 25 +++++++++++++-------- fml/unique_fd.h | 1 + shell/common/persistent_cache.cc | 15 +++++++------ 5 files changed, 70 insertions(+), 17 deletions(-) diff --git a/fml/file.h b/fml/file.h index 4df17ce2d73e9..4cd97a65b2140 100644 --- a/fml/file.h +++ b/fml/file.h @@ -73,7 +73,13 @@ bool WriteAtomically(const fml::UniqueFD& base_directory, const char* file_name, const Mapping& mapping); -std::vector ListFiles(const fml::UniqueFD& directory); +using FileVisitor = std::function; + +/// Call `visitor` on all files inside the `directory` non-recursively. If +/// recursive visiting is needed, call `VisitFiles` inside the `visitor`. +/// The trivial file "." and ".." will not be visited. +void VisitFiles(const fml::UniqueFD& directory, const FileVisitor& visitor); class ScopedTemporaryDirectory { public: diff --git a/fml/file_unittest.cc b/fml/file_unittest.cc index c492a0a36a93e..279e8e3d03d94 100644 --- a/fml/file_unittest.cc +++ b/fml/file_unittest.cc @@ -5,6 +5,7 @@ #include #include +#include "flutter/fml/unique_fd.h" #include "gtest/gtest.h" #include "flutter/fml/build_config.h" @@ -131,6 +132,43 @@ TEST(FileTest, CreateDirectoryStructure) { ASSERT_TRUE(fml::UnlinkDirectory(dir.fd(), "a")); } +TEST(FileTest, CanListFilesRecursively) { + fml::ScopedTemporaryDirectory dir; + + { + auto sub = fml::CreateDirectory(dir.fd(), {"a", "b", "c"}, + fml::FilePermission::kReadWrite); + ASSERT_TRUE(sub.is_valid()); + auto file1 = + fml::OpenFile(sub, "file1", true, fml::FilePermission::kReadWrite); + auto file2 = + fml::OpenFile(sub, "file2", true, fml::FilePermission::kReadWrite); + ASSERT_TRUE(file1.is_valid()); + ASSERT_TRUE(file2.is_valid()); + } + + std::vector names; + fml::FileVisitor visitor = [&names, &visitor](const fml::UniqueFD& directory, + const std::string& filename) { + names.push_back(filename); + fml::UniqueFD file = fml::OpenFile(directory, filename.c_str(), false, + fml::FilePermission::kRead); + if (fml::IsDirectory(file)) { + fml::VisitFiles(file, visitor); + } + }; + + fml::VisitFiles(dir.fd(), visitor); + ASSERT_EQ(names, std::vector({"a", "b", "c", "file1", "file2"})); + + // Cleanup. + ASSERT_TRUE(fml::UnlinkFile(dir.fd(), "a/b/c/file1")); + ASSERT_TRUE(fml::UnlinkFile(dir.fd(), "a/b/c/file2")); + ASSERT_TRUE(fml::UnlinkDirectory(dir.fd(), "a/b/c")); + ASSERT_TRUE(fml::UnlinkDirectory(dir.fd(), "a/b")); + ASSERT_TRUE(fml::UnlinkDirectory(dir.fd(), "a")); +} + #if OS_WIN #define AtomicWriteTest DISABLED_AtomicWriteTest #else diff --git a/fml/platform/posix/file_posix.cc b/fml/platform/posix/file_posix.cc index 28d0ba919a8f0..3154e5498d114 100644 --- a/fml/platform/posix/file_posix.cc +++ b/fml/platform/posix/file_posix.cc @@ -14,7 +14,9 @@ #include #include "flutter/fml/eintr_wrapper.h" +#include "flutter/fml/logging.h" #include "flutter/fml/mapping.h" +#include "flutter/fml/unique_fd.h" namespace fml { @@ -163,7 +165,11 @@ bool UnlinkFile(const char* path) { } bool UnlinkFile(const fml::UniqueFD& base_directory, const char* path) { - return ::unlinkat(base_directory.get(), path, 0) == 0; + int code = ::unlinkat(base_directory.get(), path, 0); + if (code != 0) { + FML_DLOG(ERROR) << strerror(errno); + } + return code == 0; } bool FileExists(const fml::UniqueFD& base_directory, const char* path) { @@ -211,20 +217,21 @@ bool WriteAtomically(const fml::UniqueFD& base_directory, base_directory.get(), file_name) == 0; } -std::vector ListFiles(const fml::UniqueFD& directory) { - std::vector files; +void VisitFiles(const fml::UniqueFD& directory, const FileVisitor& visitor) { + // We cannot call closedir(dir) because it will also close the corresponding + // UniqueFD, and later reference to that UniqueFD will fail. Also, we don't + // have to call closedir because UniqueFD will call close on its destructor. DIR* dir = ::fdopendir(directory.get()); if (dir == nullptr) { - FML_LOG(ERROR) << "Can't open the directory."; - return files; + FML_DLOG(ERROR) << "Can't open the directory. Error: " << strerror(errno); + return; } while (dirent* ent = readdir(dir)) { - std::string name = ent->d_name; - if (name != "." && name != "..") { - files.push_back(std::move(name)); + std::string filename = ent->d_name; + if (filename != "." && filename != "..") { + visitor(directory, filename); } } - return files; } } // namespace fml diff --git a/fml/unique_fd.h b/fml/unique_fd.h index 17f1a3cdaa216..52e34bfb0bb38 100644 --- a/fml/unique_fd.h +++ b/fml/unique_fd.h @@ -14,6 +14,7 @@ #else // OS_WIN +#include #include #endif // OS_WIN diff --git a/shell/common/persistent_cache.cc b/shell/common/persistent_cache.cc index 84ddb75bcfd51..f394c691968fa 100644 --- a/shell/common/persistent_cache.cc +++ b/shell/common/persistent_cache.cc @@ -102,21 +102,22 @@ std::vector PersistentCache::LoadSkSLs() { const char* path = cache_sksl_ ? "." : "sksl"; fml::UniqueFD dir = fml::OpenDirectory(*cache_directory_, path, false, fml::FilePermission::kRead); - std::vector names = fml::ListFiles(dir); - for (const std::string& name : names) { - std::pair decode_result = fml::Base32Decode(name); + fml::FileVisitor visitor = [&result](const fml::UniqueFD& directory, + const std::string& filename) { + std::pair decode_result = fml::Base32Decode(filename); if (!decode_result.first) { - FML_LOG(ERROR) << "Base32 can't decode: " << name; - continue; + FML_LOG(ERROR) << "Base32 can't decode: " << filename; + return; } const std::string& data_string = decode_result.second; sk_sp key = SkData::MakeWithCopy(data_string.data(), data_string.length()); - sk_sp data = LoadFile(dir, name); + sk_sp data = LoadFile(directory, filename); if (data != nullptr) { result.push_back({key, data}); } - } + }; + fml::VisitFiles(dir, visitor); return result; } From 5ac4685d251f1ca966a92709334809273c5808d6 Mon Sep 17 00:00:00 2001 From: Yuqian Li Date: Mon, 30 Sep 2019 10:18:32 -0700 Subject: [PATCH 3/6] Add persistent cache unit test --- fml/file.cc | 20 ++++ fml/file.h | 28 ++++- fml/file_unittest.cc | 59 ++++++++--- fml/platform/posix/file_posix.cc | 5 + fml/unique_fd.h | 1 - shell/common/BUILD.gn | 1 + shell/common/persistent_cache.cc | 16 ++- shell/common/persistent_cache.h | 5 +- shell/common/persistent_cache_unittests.cc | 115 +++++++++++++++++++++ shell/common/shell_test.cc | 14 ++- shell/common/shell_test.h | 11 +- 11 files changed, 237 insertions(+), 38 deletions(-) create mode 100644 shell/common/persistent_cache_unittests.cc diff --git a/fml/file.cc b/fml/file.cc index 8deb76c91da0f..0bc4029371eab 100644 --- a/fml/file.cc +++ b/fml/file.cc @@ -5,6 +5,7 @@ #include "flutter/fml/file.h" #include "flutter/fml/logging.h" +#include "flutter/fml/unique_fd.h" namespace fml { @@ -58,4 +59,23 @@ ScopedTemporaryDirectory::~ScopedTemporaryDirectory() { } } +void VisitFilesRecursively(const fml::UniqueFD& directory, + const FileVisitor& visitor) { + FileVisitor recursive_visitor = [&recursive_visitor, &visitor]( + const UniqueFD& directory, + const std::string& filename) { + visitor(directory, filename); + UniqueFD file = OpenFileReadOnly(directory, filename.c_str()); + if (IsDirectory(file)) { + VisitFiles(file, recursive_visitor); + } + }; + VisitFiles(directory, recursive_visitor); +} + +fml::UniqueFD OpenFileReadOnly(const fml::UniqueFD& base_directory, + const char* path) { + return OpenFile(base_directory, path, false, FilePermission::kRead); +} + } // namespace fml diff --git a/fml/file.h b/fml/file.h index 4cd97a65b2140..35e1d38362e70 100644 --- a/fml/file.h +++ b/fml/file.h @@ -37,6 +37,11 @@ fml::UniqueFD OpenFile(const fml::UniqueFD& base_directory, bool create_if_necessary, FilePermission permission); +/// Helper method that calls `OpenFile` with create_if_necessary = false +/// and permission = kRead. +fml::UniqueFD OpenFileReadOnly(const fml::UniqueFD& base_directory, + const char* path); + fml::UniqueFD OpenDirectory(const char* path, bool create_if_necessary, FilePermission permission); @@ -76,17 +81,34 @@ bool WriteAtomically(const fml::UniqueFD& base_directory, using FileVisitor = std::function; -/// Call `visitor` on all files inside the `directory` non-recursively. If -/// recursive visiting is needed, call `VisitFiles` inside the `visitor`. -/// The trivial file "." and ".." will not be visited. +/// Call `visitor` on all files inside the `directory` non-recursively. The +/// trivial file "." and ".." will not be visited. +/// +/// If recursive visiting is needed, call `VisitFiles` inside the `visitor`, or +/// use our helper method `VisitFilesRecursively`. +/// +/// @see `VisitFilesRecursively`. void VisitFiles(const fml::UniqueFD& directory, const FileVisitor& visitor); +/// Recursively call `visitor` on all files inside the `directory`. +/// +/// This is a helper method that wraps the general `VisitFiles` method. The +/// `VisitFiles` is strictly more powerful as it has the access of the recursion +/// stack to the file. For example, `VisitFiles` may be able to maintain a +/// vector of directory names that lead to a file. That could be useful to +/// compute the relative path between the root directory and the visited file. +/// +/// @see `VisitFiles`. +void VisitFilesRecursively(const fml::UniqueFD& directory, + const FileVisitor& visitor); + class ScopedTemporaryDirectory { public: ScopedTemporaryDirectory(); ~ScopedTemporaryDirectory(); + const std::string& path() const { return path_; } const UniqueFD& fd() { return dir_fd_; } private: diff --git a/fml/file_unittest.cc b/fml/file_unittest.cc index 279e8e3d03d94..7228539117beb 100644 --- a/fml/file_unittest.cc +++ b/fml/file_unittest.cc @@ -132,38 +132,65 @@ TEST(FileTest, CreateDirectoryStructure) { ASSERT_TRUE(fml::UnlinkDirectory(dir.fd(), "a")); } +TEST(FileTest, VisitFilesCanBeCalledTwice) { + fml::ScopedTemporaryDirectory dir; + + auto file = fml::OpenFile(dir.fd(), "my_contents", true, + fml::FilePermission::kReadWrite); + ASSERT_TRUE(file.is_valid()); + + int count; + fml::FileVisitor count_visitor = [&count](const fml::UniqueFD& directory, + const std::string& filename) { + count += 1; + }; + count = 0; + fml::VisitFiles(dir.fd(), count_visitor); + ASSERT_EQ(count, 1); + + // Without `rewinddir` in `VisitFiles`, the following check would fail. + count = 0; + fml::VisitFiles(dir.fd(), count_visitor); + ASSERT_EQ(count, 1); + + ASSERT_TRUE(fml::UnlinkFile(dir.fd(), "my_contents")); +} + TEST(FileTest, CanListFilesRecursively) { fml::ScopedTemporaryDirectory dir; { - auto sub = fml::CreateDirectory(dir.fd(), {"a", "b", "c"}, - fml::FilePermission::kReadWrite); - ASSERT_TRUE(sub.is_valid()); + auto c = fml::CreateDirectory(dir.fd(), {"a", "b", "c"}, + fml::FilePermission::kReadWrite); + ASSERT_TRUE(c.is_valid()); auto file1 = - fml::OpenFile(sub, "file1", true, fml::FilePermission::kReadWrite); + fml::OpenFile(c, "file1", true, fml::FilePermission::kReadWrite); auto file2 = - fml::OpenFile(sub, "file2", true, fml::FilePermission::kReadWrite); + fml::OpenFile(c, "file2", true, fml::FilePermission::kReadWrite); + auto d = fml::CreateDirectory(c, {"d"}, fml::FilePermission::kReadWrite); + ASSERT_TRUE(d.is_valid()); + auto file3 = + fml::OpenFile(d, "file3", true, fml::FilePermission::kReadWrite); ASSERT_TRUE(file1.is_valid()); ASSERT_TRUE(file2.is_valid()); + ASSERT_TRUE(file3.is_valid()); } - std::vector names; - fml::FileVisitor visitor = [&names, &visitor](const fml::UniqueFD& directory, - const std::string& filename) { - names.push_back(filename); - fml::UniqueFD file = fml::OpenFile(directory, filename.c_str(), false, - fml::FilePermission::kRead); - if (fml::IsDirectory(file)) { - fml::VisitFiles(file, visitor); - } + std::set names; + fml::FileVisitor visitor = [&names](const fml::UniqueFD& directory, + const std::string& filename) { + names.insert(filename); }; - fml::VisitFiles(dir.fd(), visitor); - ASSERT_EQ(names, std::vector({"a", "b", "c", "file1", "file2"})); + fml::VisitFilesRecursively(dir.fd(), visitor); + ASSERT_EQ(names, std::set( + {"a", "b", "c", "d", "file1", "file2", "file3"})); // Cleanup. + ASSERT_TRUE(fml::UnlinkFile(dir.fd(), "a/b/c/d/file3")); ASSERT_TRUE(fml::UnlinkFile(dir.fd(), "a/b/c/file1")); ASSERT_TRUE(fml::UnlinkFile(dir.fd(), "a/b/c/file2")); + ASSERT_TRUE(fml::UnlinkDirectory(dir.fd(), "a/b/c/d")); ASSERT_TRUE(fml::UnlinkDirectory(dir.fd(), "a/b/c")); ASSERT_TRUE(fml::UnlinkDirectory(dir.fd(), "a/b")); ASSERT_TRUE(fml::UnlinkDirectory(dir.fd(), "a")); diff --git a/fml/platform/posix/file_posix.cc b/fml/platform/posix/file_posix.cc index 3154e5498d114..3886af2db2498 100644 --- a/fml/platform/posix/file_posix.cc +++ b/fml/platform/posix/file_posix.cc @@ -226,6 +226,11 @@ void VisitFiles(const fml::UniqueFD& directory, const FileVisitor& visitor) { FML_DLOG(ERROR) << "Can't open the directory. Error: " << strerror(errno); return; } + + // Without `rewinddir`, `readir` will directly return NULL (end of dir is + // reached) after a previuos `VisitFiles` call for the same `const + // fml::UniqueFd& directory`. + rewinddir(dir); while (dirent* ent = readdir(dir)) { std::string filename = ent->d_name; if (filename != "." && filename != "..") { diff --git a/fml/unique_fd.h b/fml/unique_fd.h index 52e34bfb0bb38..17f1a3cdaa216 100644 --- a/fml/unique_fd.h +++ b/fml/unique_fd.h @@ -14,7 +14,6 @@ #else // OS_WIN -#include #include #endif // OS_WIN diff --git a/shell/common/BUILD.gn b/shell/common/BUILD.gn index b3745aef2f262..01dc56517c263 100644 --- a/shell/common/BUILD.gn +++ b/shell/common/BUILD.gn @@ -158,6 +158,7 @@ if (current_toolchain == host_toolchain) { shell_host_executable("shell_unittests") { sources = [ "canvas_spy_unittests.cc", + "persistent_cache_unittests.cc", "input_events_unittests.cc", "pipeline_unittests.cc", "shell_test.cc", diff --git a/shell/common/persistent_cache.cc b/shell/common/persistent_cache.cc index f394c691968fa..d0bec5890d481 100644 --- a/shell/common/persistent_cache.cc +++ b/shell/common/persistent_cache.cc @@ -99,9 +99,6 @@ std::vector PersistentCache::LoadSkSLs() { if (!IsValid()) { return result; } - const char* path = cache_sksl_ ? "." : "sksl"; - fml::UniqueFD dir = fml::OpenDirectory(*cache_directory_, path, false, - fml::FilePermission::kRead); fml::FileVisitor visitor = [&result](const fml::UniqueFD& directory, const std::string& filename) { std::pair decode_result = fml::Base32Decode(filename); @@ -117,14 +114,15 @@ std::vector PersistentCache::LoadSkSLs() { result.push_back({key, data}); } }; - fml::VisitFiles(dir, visitor); + fml::VisitFiles(*sksl_cache_directory_, visitor); return result; } PersistentCache::PersistentCache(bool read_only) : is_read_only_(read_only), - cache_directory_( - MakeCacheDirectory(cache_base_path_, read_only, cache_sksl_)) { + cache_directory_(MakeCacheDirectory(cache_base_path_, read_only, false)), + sksl_cache_directory_( + MakeCacheDirectory(cache_base_path_, read_only, true)) { if (!IsValid()) { FML_LOG(WARNING) << "Could not acquire the persistent cache directory. " "Caching of GPU resources on disk is disabled."; @@ -139,8 +137,7 @@ bool PersistentCache::IsValid() const { sk_sp PersistentCache::LoadFile(const fml::UniqueFD& dir, const std::string& file_name) { - auto file = - fml::OpenFile(dir, file_name.c_str(), false, fml::FilePermission::kRead); + auto file = fml::OpenFileReadOnly(dir, file_name.c_str()); if (!file.is_valid()) { return nullptr; } @@ -225,7 +222,8 @@ void PersistentCache::store(const SkData& key, const SkData& data) { return; } - PersistentCacheStore(GetWorkerTaskRunner(), cache_directory_, + PersistentCacheStore(GetWorkerTaskRunner(), + cache_sksl_ ? sksl_cache_directory_ : cache_directory_, std::move(file_name), std::move(mapping)); } diff --git a/shell/common/persistent_cache.h b/shell/common/persistent_cache.h index fcc7783922f1d..c8e1df823ea79 100644 --- a/shell/common/persistent_cache.h +++ b/shell/common/persistent_cache.h @@ -49,10 +49,6 @@ class PersistentCache : public GrContextOptions::PersistentCache { bool IsDumpingSkp() const { return is_dumping_skp_; } void SetIsDumpingSkp(bool value) { is_dumping_skp_ = value; } - // Return a list of file names inside the sksl cache directory. The name is - // the Base32 encoding of the SkSL key (of type SkData). - std::vector ListSkSL() const; - // |GrContextOptions::PersistentCache| sk_sp load(const SkData& key) override; @@ -80,6 +76,7 @@ class PersistentCache : public GrContextOptions::PersistentCache { const bool is_read_only_; const std::shared_ptr cache_directory_; + const std::shared_ptr sksl_cache_directory_; mutable std::mutex worker_task_runners_mutex_; std::multiset> worker_task_runners_ FML_GUARDED_BY(worker_task_runners_mutex_); diff --git a/shell/common/persistent_cache_unittests.cc b/shell/common/persistent_cache_unittests.cc new file mode 100644 index 0000000000000..f5c6b9abcdfa5 --- /dev/null +++ b/shell/common/persistent_cache_unittests.cc @@ -0,0 +1,115 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include +#include "flutter/flow/layers/container_layer.h" +#include "flutter/flow/layers/layer.h" +#include "flutter/flow/layers/physical_shape_layer.h" +#include "flutter/flow/layers/picture_layer.h" +#include "flutter/fml/command_line.h" +#include "flutter/fml/file.h" +#include "flutter/fml/unique_fd.h" +#include "flutter/shell/common/persistent_cache.h" +#include "flutter/shell/common/shell_test.h" +#include "flutter/shell/common/switches.h" +#include "flutter/testing/testing.h" +#include "include/core/SkPicture.h" + +namespace flutter { +namespace testing { + +static void WaitForIO(Shell* shell) { + std::promise io_task_finished; + shell->GetTaskRunners().GetIOTaskRunner()->PostTask( + [&io_task_finished]() { io_task_finished.set_value(true); }); + io_task_finished.get_future().wait(); +} + +TEST_F(ShellTest, CacheSkSLWorks) { + // Create a temp dir to store the persistent cache + fml::ScopedTemporaryDirectory dir; + PersistentCache::SetCacheDirectoryPath(dir.path()); + + auto settings = CreateSettingsForFixture(); + settings.cache_sksl = true; + settings.dump_skp_on_shader_compilation = true; + auto sksl_config = RunConfiguration::InferFromSettings(settings); + sksl_config.SetEntrypoint("emptyMain"); + std::unique_ptr shell = CreateShell(settings); + PlatformViewNotifyCreated(shell.get()); + RunEngine(shell.get(), std::move(sksl_config)); + + // Initially, we should have no SkSL cache + auto cache = PersistentCache::GetCacheForProcess()->LoadSkSLs(); + ASSERT_EQ(cache.size(), 0u); + + // Draw something to trigger shader compilations. + LayerTreeBuilder builder = [](std::shared_ptr root) { + SkPath path; + path.addCircle(50, 50, 20); + auto physical_shape_layer = std::make_shared( + SK_ColorRED, SK_ColorBLUE, 1.0, 1.0, 1.0, path, Clip::antiAlias); + root->Add(physical_shape_layer); + }; + PumpOneFrame(shell.get(), 100, 100, builder); + fml::Status result = + shell->WaitForFirstFrame(fml::TimeDelta::FromMilliseconds(1000)); + ASSERT_TRUE(result.ok()); + WaitForIO(shell.get()); + + // Some skp should be dumped due to shader compilations. + int skp_count = 0; + fml::FileVisitor skp_visitor = [&skp_count](const fml::UniqueFD& directory, + const std::string& filename) { + if (filename.size() >= 4 && + filename.substr(filename.size() - 4, 4) == ".skp") { + skp_count += 1; + } + }; + fml::VisitFilesRecursively(dir.fd(), skp_visitor); + ASSERT_GT(skp_count, 0); + + // SkSL cache should be generated by the last run. + cache = PersistentCache::GetCacheForProcess()->LoadSkSLs(); + ASSERT_GT(cache.size(), 0u); + + // Run the engine again with cache_sksl = false and check that the previously + // generated SkSL cache is used for precompile. + settings.cache_sksl = false; + settings.dump_skp_on_shader_compilation = true; + auto normal_config = RunConfiguration::InferFromSettings(settings); + normal_config.SetEntrypoint("emptyMain"); + shell.reset(); + shell = CreateShell(settings); + PlatformViewNotifyCreated(shell.get()); + RunEngine(shell.get(), std::move(normal_config)); + PumpOneFrame(shell.get(), 100, 100, builder); + result = shell->WaitForFirstFrame(fml::TimeDelta::FromMilliseconds(1000)); + ASSERT_TRUE(result.ok()); + WaitForIO(shell.get()); + + // To check that all shaders are precompiled, verify that no new skp is dumped + // due to shader compilations. + int old_skp_count = skp_count; + skp_count = 0; + fml::VisitFilesRecursively(dir.fd(), skp_visitor); + ASSERT_EQ(skp_count, old_skp_count); + + // Remove all files generated + fml::FileVisitor remove_visitor = [&remove_visitor]( + const fml::UniqueFD& directory, + const std::string& filename) { + fml::UniqueFD file = fml::OpenFileReadOnly(directory, filename.c_str()); + if (fml::IsDirectory(file)) { + fml::VisitFiles(file, remove_visitor); + fml::UnlinkDirectory(directory, filename.c_str()); + } else { + fml::UnlinkFile(directory, filename.c_str()); + } + }; + fml::VisitFiles(dir.fd(), remove_visitor); +} + +} // namespace testing +} // namespace flutter diff --git a/shell/common/shell_test.cc b/shell/common/shell_test.cc index 1e7d7c4b1d8c6..fa937a86c5626 100644 --- a/shell/common/shell_test.cc +++ b/shell/common/shell_test.cc @@ -135,15 +135,18 @@ void ShellTest::VSyncFlush(Shell* shell, bool& will_draw_new_frame) { latch.Wait(); } -void ShellTest::PumpOneFrame(Shell* shell) { +void ShellTest::PumpOneFrame(Shell* shell, + double width, + double height, + LayerTreeBuilder builder) { // Set viewport to nonempty, and call Animator::BeginFrame to make the layer // tree pipeline nonempty. Without either of this, the layer tree below // won't be rasterized. fml::AutoResetWaitableEvent latch; shell->GetTaskRunners().GetUITaskRunner()->PostTask( - [&latch, engine = shell->weak_engine_]() { + [&latch, engine = shell->weak_engine_, width, height]() { engine->SetViewportMetrics( - {1, 1, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}); + {1, width, height, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}); engine->animator_->BeginFrame(fml::TimePoint::Now(), fml::TimePoint::Now()); latch.Signal(); @@ -154,12 +157,15 @@ void ShellTest::PumpOneFrame(Shell* shell) { // Call |Render| to rasterize a layer tree and trigger |OnFrameRasterized| fml::WeakPtr runtime_delegate = shell->weak_engine_; shell->GetTaskRunners().GetUITaskRunner()->PostTask( - [&latch, runtime_delegate]() { + [&latch, runtime_delegate, &builder]() { auto layer_tree = std::make_unique(); SkMatrix identity; identity.setIdentity(); auto root_layer = std::make_shared(identity); layer_tree->set_root_layer(root_layer); + if (builder) { + builder(root_layer); + } runtime_delegate->Render(std::move(layer_tree)); latch.Signal(); }); diff --git a/shell/common/shell_test.h b/shell/common/shell_test.h index ca486230a42c6..794c499a84bd0 100644 --- a/shell/common/shell_test.h +++ b/shell/common/shell_test.h @@ -8,6 +8,7 @@ #include #include "flutter/common/settings.h" +#include "flutter/flow/layers/container_layer.h" #include "flutter/fml/macros.h" #include "flutter/fml/synchronization/thread_annotations.h" #include "flutter/lib/ui/window/platform_message.h" @@ -50,7 +51,15 @@ class ShellTest : public ThreadTest { /// the `will_draw_new_frame` to true. static void VSyncFlush(Shell* shell, bool& will_draw_new_frame); - static void PumpOneFrame(Shell* shell); + /// Given the root layer, this callback builds the layer tree to be rasterized + /// in PumpOneFrame. + using LayerTreeBuilder = + std::function root)>; + static void PumpOneFrame(Shell* shell, + double width = 1, + double height = 1, + LayerTreeBuilder = {}); + static void DispatchFakePointerData(Shell* shell); // Declare |UnreportedTimingsCount|, |GetNeedsReportTimings| and From 940e80f46cdb68ab613a01f63ae883bd6a515ac5 Mon Sep 17 00:00:00 2001 From: Yuqian Li Date: Wed, 2 Oct 2019 12:31:28 -0700 Subject: [PATCH 4/6] Fxies according to tests and comments --- ci/licenses_golden/licenses_flutter | 1 + fml/file.cc | 15 ++++++++---- fml/file.h | 18 +++++++++++---- fml/file_unittest.cc | 27 ++++++++++++++++++++++ fml/platform/posix/file_posix.cc | 9 +++++--- shell/common/BUILD.gn | 2 +- shell/common/persistent_cache.cc | 5 +++- shell/common/persistent_cache_unittests.cc | 2 ++ 8 files changed, 64 insertions(+), 15 deletions(-) diff --git a/ci/licenses_golden/licenses_flutter b/ci/licenses_golden/licenses_flutter index cccbe6a7537c7..8bbc29d280cea 100644 --- a/ci/licenses_golden/licenses_flutter +++ b/ci/licenses_golden/licenses_flutter @@ -492,6 +492,7 @@ FILE: ../../../flutter/shell/common/isolate_configuration.cc FILE: ../../../flutter/shell/common/isolate_configuration.h FILE: ../../../flutter/shell/common/persistent_cache.cc FILE: ../../../flutter/shell/common/persistent_cache.h +FILE: ../../../flutter/shell/common/persistent_cache_unittests.cc FILE: ../../../flutter/shell/common/pipeline.cc FILE: ../../../flutter/shell/common/pipeline.h FILE: ../../../flutter/shell/common/pipeline_unittests.cc diff --git a/fml/file.cc b/fml/file.cc index 0bc4029371eab..435c09f027544 100644 --- a/fml/file.cc +++ b/fml/file.cc @@ -59,18 +59,23 @@ ScopedTemporaryDirectory::~ScopedTemporaryDirectory() { } } -void VisitFilesRecursively(const fml::UniqueFD& directory, - const FileVisitor& visitor) { +bool VisitFilesRecursively(const fml::UniqueFD& directory, + FileVisitor visitor) { FileVisitor recursive_visitor = [&recursive_visitor, &visitor]( const UniqueFD& directory, const std::string& filename) { - visitor(directory, filename); + if (!visitor(directory, filename)) { + return false; + } UniqueFD file = OpenFileReadOnly(directory, filename.c_str()); if (IsDirectory(file)) { - VisitFiles(file, recursive_visitor); + if (!VisitFiles(file, recursive_visitor)) { + return false; + } } + return true; }; - VisitFiles(directory, recursive_visitor); + return VisitFiles(directory, recursive_visitor); } fml::UniqueFD OpenFileReadOnly(const fml::UniqueFD& base_directory, diff --git a/fml/file.h b/fml/file.h index 35e1d38362e70..d343edb149530 100644 --- a/fml/file.h +++ b/fml/file.h @@ -5,6 +5,7 @@ #ifndef FLUTTER_FML_FILE_H_ #define FLUTTER_FML_FILE_H_ +#include #include #include #include @@ -78,19 +79,27 @@ bool WriteAtomically(const fml::UniqueFD& base_directory, const char* file_name, const Mapping& mapping); -using FileVisitor = std::function; /// Call `visitor` on all files inside the `directory` non-recursively. The /// trivial file "." and ".." will not be visited. /// +/// Return false if and only if the visitor returns false during the +/// traversal. +/// /// If recursive visiting is needed, call `VisitFiles` inside the `visitor`, or /// use our helper method `VisitFilesRecursively`. /// /// @see `VisitFilesRecursively`. -void VisitFiles(const fml::UniqueFD& directory, const FileVisitor& visitor); +bool VisitFiles(const fml::UniqueFD& directory, FileVisitor visitor); -/// Recursively call `visitor` on all files inside the `directory`. +/// Recursively call `visitor` on all files inside the `directory`. Return false +/// if and only if the visitor returns false during the traversal. /// /// This is a helper method that wraps the general `VisitFiles` method. The /// `VisitFiles` is strictly more powerful as it has the access of the recursion @@ -99,8 +108,7 @@ void VisitFiles(const fml::UniqueFD& directory, const FileVisitor& visitor); /// compute the relative path between the root directory and the visited file. /// /// @see `VisitFiles`. -void VisitFilesRecursively(const fml::UniqueFD& directory, - const FileVisitor& visitor); +bool VisitFilesRecursively(const fml::UniqueFD& directory, FileVisitor visitor); class ScopedTemporaryDirectory { public: diff --git a/fml/file_unittest.cc b/fml/file_unittest.cc index 7228539117beb..f856b50e2f066 100644 --- a/fml/file_unittest.cc +++ b/fml/file_unittest.cc @@ -143,6 +143,7 @@ TEST(FileTest, VisitFilesCanBeCalledTwice) { fml::FileVisitor count_visitor = [&count](const fml::UniqueFD& directory, const std::string& filename) { count += 1; + return true; }; count = 0; fml::VisitFiles(dir.fd(), count_visitor); @@ -180,6 +181,7 @@ TEST(FileTest, CanListFilesRecursively) { fml::FileVisitor visitor = [&names](const fml::UniqueFD& directory, const std::string& filename) { names.insert(filename); + return true; }; fml::VisitFilesRecursively(dir.fd(), visitor); @@ -196,6 +198,31 @@ TEST(FileTest, CanListFilesRecursively) { ASSERT_TRUE(fml::UnlinkDirectory(dir.fd(), "a")); } +TEST(FileTest, CanStopVisitEarly) { + fml::ScopedTemporaryDirectory dir; + + auto d = fml::CreateDirectory(dir.fd(), {"a", "b", "c", "d"}, + fml::FilePermission::kReadWrite); + ASSERT_TRUE(d.is_valid()); + + std::set names; + fml::FileVisitor visitor = [&names](const fml::UniqueFD& directory, + const std::string& filename) { + names.insert(filename); + return filename == "c" ? false : true; // stop if c is found + }; + + // Check the d is not visited as we stop at c. + ASSERT_FALSE(fml::VisitFilesRecursively(dir.fd(), visitor)); + ASSERT_EQ(names, std::set({"a", "b", "c"})); + + // Cleanup. + ASSERT_TRUE(fml::UnlinkDirectory(dir.fd(), "a/b/c/d")); + ASSERT_TRUE(fml::UnlinkDirectory(dir.fd(), "a/b/c")); + ASSERT_TRUE(fml::UnlinkDirectory(dir.fd(), "a/b")); + ASSERT_TRUE(fml::UnlinkDirectory(dir.fd(), "a")); +} + #if OS_WIN #define AtomicWriteTest DISABLED_AtomicWriteTest #else diff --git a/fml/platform/posix/file_posix.cc b/fml/platform/posix/file_posix.cc index 3886af2db2498..335182c5a1ece 100644 --- a/fml/platform/posix/file_posix.cc +++ b/fml/platform/posix/file_posix.cc @@ -217,14 +217,14 @@ bool WriteAtomically(const fml::UniqueFD& base_directory, base_directory.get(), file_name) == 0; } -void VisitFiles(const fml::UniqueFD& directory, const FileVisitor& visitor) { +bool VisitFiles(const fml::UniqueFD& directory, FileVisitor visitor) { // We cannot call closedir(dir) because it will also close the corresponding // UniqueFD, and later reference to that UniqueFD will fail. Also, we don't // have to call closedir because UniqueFD will call close on its destructor. DIR* dir = ::fdopendir(directory.get()); if (dir == nullptr) { FML_DLOG(ERROR) << "Can't open the directory. Error: " << strerror(errno); - return; + return true; // continue to visit other files } // Without `rewinddir`, `readir` will directly return NULL (end of dir is @@ -234,9 +234,12 @@ void VisitFiles(const fml::UniqueFD& directory, const FileVisitor& visitor) { while (dirent* ent = readdir(dir)) { std::string filename = ent->d_name; if (filename != "." && filename != "..") { - visitor(directory, filename); + if (!visitor(directory, filename)) { + return false; + } } } + return true; } } // namespace fml diff --git a/shell/common/BUILD.gn b/shell/common/BUILD.gn index 01dc56517c263..f93bca63478f0 100644 --- a/shell/common/BUILD.gn +++ b/shell/common/BUILD.gn @@ -158,8 +158,8 @@ if (current_toolchain == host_toolchain) { shell_host_executable("shell_unittests") { sources = [ "canvas_spy_unittests.cc", - "persistent_cache_unittests.cc", "input_events_unittests.cc", + "persistent_cache_unittests.cc", "pipeline_unittests.cc", "shell_test.cc", "shell_test.h", diff --git a/shell/common/persistent_cache.cc b/shell/common/persistent_cache.cc index d0bec5890d481..5e540f1e6d03a 100644 --- a/shell/common/persistent_cache.cc +++ b/shell/common/persistent_cache.cc @@ -104,7 +104,7 @@ std::vector PersistentCache::LoadSkSLs() { std::pair decode_result = fml::Base32Decode(filename); if (!decode_result.first) { FML_LOG(ERROR) << "Base32 can't decode: " << filename; - return; + return true; // continue to visit other files } const std::string& data_string = decode_result.second; sk_sp key = @@ -112,7 +112,10 @@ std::vector PersistentCache::LoadSkSLs() { sk_sp data = LoadFile(directory, filename); if (data != nullptr) { result.push_back({key, data}); + } else { + FML_LOG(ERROR) << "Failed to load: " << filename; } + return true; }; fml::VisitFiles(*sksl_cache_directory_, visitor); return result; diff --git a/shell/common/persistent_cache_unittests.cc b/shell/common/persistent_cache_unittests.cc index f5c6b9abcdfa5..1bcc213e45295 100644 --- a/shell/common/persistent_cache_unittests.cc +++ b/shell/common/persistent_cache_unittests.cc @@ -66,6 +66,7 @@ TEST_F(ShellTest, CacheSkSLWorks) { filename.substr(filename.size() - 4, 4) == ".skp") { skp_count += 1; } + return true; }; fml::VisitFilesRecursively(dir.fd(), skp_visitor); ASSERT_GT(skp_count, 0); @@ -107,6 +108,7 @@ TEST_F(ShellTest, CacheSkSLWorks) { } else { fml::UnlinkFile(directory, filename.c_str()); } + return true; }; fml::VisitFiles(dir.fd(), remove_visitor); } From 2c41d1d974635be4884d870fb62ed639567cdfa9 Mon Sep 17 00:00:00 2001 From: Yuqian Li Date: Fri, 4 Oct 2019 14:41:25 -0700 Subject: [PATCH 5/6] Try to fix windows build --- fml/file.cc | 17 +++++++++--- fml/file.h | 11 ++++++++ fml/file_unittest.cc | 32 +++++++++++++--------- fml/platform/posix/file_posix.cc | 5 ++++ fml/platform/win/file_win.cc | 31 +++++++++++++++++++++ shell/common/persistent_cache_unittests.cc | 12 +++++--- 6 files changed, 87 insertions(+), 21 deletions(-) diff --git a/fml/file.cc b/fml/file.cc index 435c09f027544..2f62e6c6f884b 100644 --- a/fml/file.cc +++ b/fml/file.cc @@ -52,6 +52,8 @@ ScopedTemporaryDirectory::ScopedTemporaryDirectory() { } ScopedTemporaryDirectory::~ScopedTemporaryDirectory() { + // Windows has to close UniqueFD first before UnlinkDirectory + dir_fd_.reset(); if (path_ != "") { if (!UnlinkDirectory(path_.c_str())) { FML_LOG(ERROR) << "Could not remove directory: " << path_; @@ -67,11 +69,13 @@ bool VisitFilesRecursively(const fml::UniqueFD& directory, if (!visitor(directory, filename)) { return false; } - UniqueFD file = OpenFileReadOnly(directory, filename.c_str()); - if (IsDirectory(file)) { - if (!VisitFiles(file, recursive_visitor)) { - return false; + if (IsDirectory(directory, filename.c_str())) { + UniqueFD sub_dir = OpenDirectoryReadOnly(directory, filename.c_str()); + if (!sub_dir.is_valid()) { + FML_LOG(ERROR) << "Can't open sub-directory: " << filename; + return true; } + return VisitFiles(sub_dir, recursive_visitor); } return true; }; @@ -83,4 +87,9 @@ fml::UniqueFD OpenFileReadOnly(const fml::UniqueFD& base_directory, return OpenFile(base_directory, path, false, FilePermission::kRead); } +fml::UniqueFD OpenDirectoryReadOnly(const fml::UniqueFD& base_directory, + const char* path) { + return OpenDirectory(base_directory, path, false, FilePermission::kRead); +} + } // namespace fml diff --git a/fml/file.h b/fml/file.h index d343edb149530..ee54354520695 100644 --- a/fml/file.h +++ b/fml/file.h @@ -29,10 +29,12 @@ enum class FilePermission { std::string CreateTemporaryDirectory(); +/// This can open a directory on POSIX, but not on Windows. fml::UniqueFD OpenFile(const char* path, bool create_if_necessary, FilePermission permission); +/// This can open a directory on POSIX, but not on Windows. fml::UniqueFD OpenFile(const fml::UniqueFD& base_directory, const char* path, bool create_if_necessary, @@ -40,6 +42,8 @@ fml::UniqueFD OpenFile(const fml::UniqueFD& base_directory, /// Helper method that calls `OpenFile` with create_if_necessary = false /// and permission = kRead. +/// +/// This can open a directory on POSIX, but not on Windows. fml::UniqueFD OpenFileReadOnly(const fml::UniqueFD& base_directory, const char* path); @@ -52,10 +56,17 @@ fml::UniqueFD OpenDirectory(const fml::UniqueFD& base_directory, bool create_if_necessary, FilePermission permission); +/// Helper method that calls `OpenDirectory` with create_if_necessary = false +/// and permission = kRead. +fml::UniqueFD OpenDirectoryReadOnly(const fml::UniqueFD& base_directory, + const char* path); + fml::UniqueFD Duplicate(fml::UniqueFD::element_type descriptor); bool IsDirectory(const fml::UniqueFD& directory); +bool IsDirectory(const fml::UniqueFD& base_directory, const char* path); + // Returns whether the given path is a file. bool IsFile(const std::string& path); diff --git a/fml/file_unittest.cc b/fml/file_unittest.cc index f856b50e2f066..aed268b3fc7b2 100644 --- a/fml/file_unittest.cc +++ b/fml/file_unittest.cc @@ -5,12 +5,12 @@ #include #include -#include "flutter/fml/unique_fd.h" #include "gtest/gtest.h" #include "flutter/fml/build_config.h" #include "flutter/fml/file.h" #include "flutter/fml/mapping.h" +#include "flutter/fml/unique_fd.h" static bool WriteStringToFile(const fml::UniqueFD& fd, const std::string& contents) { @@ -135,9 +135,11 @@ TEST(FileTest, CreateDirectoryStructure) { TEST(FileTest, VisitFilesCanBeCalledTwice) { fml::ScopedTemporaryDirectory dir; - auto file = fml::OpenFile(dir.fd(), "my_contents", true, - fml::FilePermission::kReadWrite); - ASSERT_TRUE(file.is_valid()); + { + auto file = fml::OpenFile(dir.fd(), "my_contents", true, + fml::FilePermission::kReadWrite); + ASSERT_TRUE(file.is_valid()); + } int count; fml::FileVisitor count_visitor = [&count](const fml::UniqueFD& directory, @@ -201,9 +203,11 @@ TEST(FileTest, CanListFilesRecursively) { TEST(FileTest, CanStopVisitEarly) { fml::ScopedTemporaryDirectory dir; - auto d = fml::CreateDirectory(dir.fd(), {"a", "b", "c", "d"}, - fml::FilePermission::kReadWrite); - ASSERT_TRUE(d.is_valid()); + { + auto d = fml::CreateDirectory(dir.fd(), {"a", "b", "c", "d"}, + fml::FilePermission::kReadWrite); + ASSERT_TRUE(d.is_valid()); + } std::set names; fml::FileVisitor visitor = [&names](const fml::UniqueFD& directory, @@ -251,13 +255,15 @@ TEST(FileTest, AtomicWriteTest) { TEST(FileTest, EmptyMappingTest) { fml::ScopedTemporaryDirectory dir; - auto file = fml::OpenFile(dir.fd(), "my_contents", true, - fml::FilePermission::kReadWrite); + { + auto file = fml::OpenFile(dir.fd(), "my_contents", true, + fml::FilePermission::kReadWrite); - fml::FileMapping mapping(file); - ASSERT_TRUE(mapping.IsValid()); - ASSERT_EQ(mapping.GetSize(), 0ul); - ASSERT_EQ(mapping.GetMapping(), nullptr); + fml::FileMapping mapping(file); + ASSERT_TRUE(mapping.IsValid()); + ASSERT_EQ(mapping.GetSize(), 0ul); + ASSERT_EQ(mapping.GetMapping(), nullptr); + } ASSERT_TRUE(fml::UnlinkFile(dir.fd(), "my_contents")); } diff --git a/fml/platform/posix/file_posix.cc b/fml/platform/posix/file_posix.cc index 335182c5a1ece..516565e9f7673 100644 --- a/fml/platform/posix/file_posix.cc +++ b/fml/platform/posix/file_posix.cc @@ -135,6 +135,11 @@ bool IsDirectory(const fml::UniqueFD& directory) { return S_ISDIR(stat_result.st_mode); } +bool IsDirectory(const fml::UniqueFD& base_directory, const char* path) { + UniqueFD file = OpenFileReadOnly(base_directory, path); + return (file.is_valid() && IsDirectory(file)); +} + bool IsFile(const std::string& path) { struct stat buf; if (stat(path.c_str(), &buf) != 0) { diff --git a/fml/platform/win/file_win.cc b/fml/platform/win/file_win.cc index bad9d052fb942..42a5958f4d061 100644 --- a/fml/platform/win/file_win.cc +++ b/fml/platform/win/file_win.cc @@ -252,6 +252,12 @@ bool IsDirectory(const fml::UniqueFD& directory) { return info.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY; } +bool IsDirectory(const fml::UniqueFD& base_directory, const char* path) { + std::string full_path = GetFullHandlePath(base_directory) + "\\" + path; + return ::GetFileAttributes(ConvertToWString(full_path.c_str()).c_str()) & + FILE_ATTRIBUTE_DIRECTORY; +} + bool IsFile(const std::string& path) { struct stat buf; if (stat(path.c_str(), &buf) != 0) @@ -393,4 +399,29 @@ bool WriteAtomically(const fml::UniqueFD& base_directory, return true; } +bool VisitFiles(const fml::UniqueFD& directory, FileVisitor visitor) { + std::string search_pattern = GetFullHandlePath(directory) + "\\*"; + WIN32_FIND_DATA find_file_data; + HANDLE find_handle = ::FindFirstFile( + StringToWideString(search_pattern).c_str(), &find_file_data); + + if (find_handle == INVALID_HANDLE_VALUE) { + FML_DLOG(ERROR) << "Can't open the directory. Error: " + << GetLastErrorMessage(); + return true; // continue to visit other files + } + + do { + std::string filename = WideStringToString(find_file_data.cFileName); + if (filename != "." && filename != "..") { + if (!visitor(directory, filename)) { + ::FindClose(find_handle); + return false; + } + } + } while (::FindNextFile(find_handle, &find_file_data)); + ::FindClose(find_handle); + return true; +} + } // namespace fml diff --git a/shell/common/persistent_cache_unittests.cc b/shell/common/persistent_cache_unittests.cc index 1bcc213e45295..0145fdf8e0787 100644 --- a/shell/common/persistent_cache_unittests.cc +++ b/shell/common/persistent_cache_unittests.cc @@ -3,6 +3,7 @@ // found in the LICENSE file. #include + #include "flutter/flow/layers/container_layer.h" #include "flutter/flow/layers/layer.h" #include "flutter/flow/layers/physical_shape_layer.h" @@ -49,7 +50,7 @@ TEST_F(ShellTest, CacheSkSLWorks) { SkPath path; path.addCircle(50, 50, 20); auto physical_shape_layer = std::make_shared( - SK_ColorRED, SK_ColorBLUE, 1.0, 1.0, 1.0, path, Clip::antiAlias); + SK_ColorRED, SK_ColorBLUE, 1.0f, 1.0f, 1.0f, path, Clip::antiAlias); root->Add(physical_shape_layer); }; PumpOneFrame(shell.get(), 100, 100, builder); @@ -101,9 +102,12 @@ TEST_F(ShellTest, CacheSkSLWorks) { fml::FileVisitor remove_visitor = [&remove_visitor]( const fml::UniqueFD& directory, const std::string& filename) { - fml::UniqueFD file = fml::OpenFileReadOnly(directory, filename.c_str()); - if (fml::IsDirectory(file)) { - fml::VisitFiles(file, remove_visitor); + if (fml::IsDirectory(directory, filename.c_str())) { + { // To trigger fml::~UniqueFD before fml::UnlinkDirectory + fml::UniqueFD sub_dir = + fml::OpenDirectoryReadOnly(directory, filename.c_str()); + fml::VisitFiles(sub_dir, remove_visitor); + } fml::UnlinkDirectory(directory, filename.c_str()); } else { fml::UnlinkFile(directory, filename.c_str()); From 5bf0c7bb9b7126fc533057ada90dfd2b0b1b5970 Mon Sep 17 00:00:00 2001 From: Yuqian Li Date: Fri, 4 Oct 2019 16:42:14 -0700 Subject: [PATCH 6/6] Enable PersistentCache reset Otherwise, the unit test would fail if previous unit tests have already set the PersistentCache with different settings. --- shell/common/persistent_cache.cc | 17 +++++++++++++---- shell/common/persistent_cache.h | 7 ++++++- shell/common/persistent_cache_unittests.cc | 2 ++ shell/common/shell.cc | 4 ++-- 4 files changed, 23 insertions(+), 7 deletions(-) diff --git a/shell/common/persistent_cache.cc b/shell/common/persistent_cache.cc index 5e540f1e6d03a..23308fe4fa228 100644 --- a/shell/common/persistent_cache.cc +++ b/shell/common/persistent_cache.cc @@ -21,6 +21,9 @@ namespace flutter { std::string PersistentCache::cache_base_path_; +std::mutex PersistentCache::instance_mutex_; +std::unique_ptr PersistentCache::gPersistentCache; + static std::string SkKeyToFilePath(const SkData& data) { if (data.data() == nullptr || data.size() == 0) { return ""; @@ -53,13 +56,19 @@ void PersistentCache::SetCacheSkSL(bool value) { } PersistentCache* PersistentCache::GetCacheForProcess() { - static std::unique_ptr gPersistentCache; - static std::once_flag once = {}; - std::call_once( - once, []() { gPersistentCache.reset(new PersistentCache(gIsReadOnly)); }); + std::scoped_lock lock(instance_mutex_); + if (gPersistentCache == nullptr) { + gPersistentCache.reset(new PersistentCache(gIsReadOnly)); + } return gPersistentCache.get(); } +void PersistentCache::ResetCacheForProcess() { + std::scoped_lock lock(instance_mutex_); + gPersistentCache.reset(new PersistentCache(gIsReadOnly)); + strategy_set_ = false; +} + void PersistentCache::SetCacheDirectoryPath(std::string path) { cache_base_path_ = path; } diff --git a/shell/common/persistent_cache.h b/shell/common/persistent_cache.h index c8e1df823ea79..a01368cb498cb 100644 --- a/shell/common/persistent_cache.h +++ b/shell/common/persistent_cache.h @@ -31,6 +31,7 @@ class PersistentCache : public GrContextOptions::PersistentCache { static bool gIsReadOnly; static PersistentCache* GetCacheForProcess(); + static void ResetCacheForProcess(); static void SetCacheDirectoryPath(std::string path); @@ -64,6 +65,10 @@ class PersistentCache : public GrContextOptions::PersistentCache { private: static std::string cache_base_path_; + static std::mutex instance_mutex_; + static std::unique_ptr gPersistentCache + FML_GUARDED_BY(instance_mutex_); + // Mutable static switch that can be set before GetCacheForProcess is called // and GrContextOptions.fShaderCacheStrategy is set. If true, it means that // we'll set `GrContextOptions::fShaderCacheStrategy` to `kSkSL`, and all the @@ -71,7 +76,7 @@ class PersistentCache : public GrContextOptions::PersistentCache { static std::atomic cache_sksl_; // Guard flag to make sure that cache_sksl_ is not modified after - // cache_strategy_set_ becomes true. + // strategy_set_ becomes true. static std::atomic strategy_set_; const bool is_read_only_; diff --git a/shell/common/persistent_cache_unittests.cc b/shell/common/persistent_cache_unittests.cc index 0145fdf8e0787..80257cdbde01f 100644 --- a/shell/common/persistent_cache_unittests.cc +++ b/shell/common/persistent_cache_unittests.cc @@ -31,6 +31,7 @@ TEST_F(ShellTest, CacheSkSLWorks) { // Create a temp dir to store the persistent cache fml::ScopedTemporaryDirectory dir; PersistentCache::SetCacheDirectoryPath(dir.path()); + PersistentCache::ResetCacheForProcess(); auto settings = CreateSettingsForFixture(); settings.cache_sksl = true; @@ -78,6 +79,7 @@ TEST_F(ShellTest, CacheSkSLWorks) { // Run the engine again with cache_sksl = false and check that the previously // generated SkSL cache is used for precompile. + PersistentCache::ResetCacheForProcess(); settings.cache_sksl = false; settings.dump_skp_on_shader_compilation = true; auto normal_config = RunConfiguration::InferFromSettings(settings); diff --git a/shell/common/shell.cc b/shell/common/shell.cc index a0e8a5be55b3a..d31b0dcc3a8b6 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -199,8 +199,6 @@ static void PerformInitializationTasks(const Settings& settings) { FML_DLOG(WARNING) << "Skipping ICU initialization in the shell."; } } - - PersistentCache::SetCacheSkSL(settings.cache_sksl); }); } @@ -210,6 +208,7 @@ std::unique_ptr Shell::Create( Shell::CreateCallback on_create_platform_view, Shell::CreateCallback on_create_rasterizer) { PerformInitializationTasks(settings); + PersistentCache::SetCacheSkSL(settings.cache_sksl); TRACE_EVENT0("flutter", "Shell::Create"); @@ -237,6 +236,7 @@ std::unique_ptr Shell::Create( Shell::CreateCallback on_create_rasterizer, DartVMRef vm) { PerformInitializationTasks(settings); + PersistentCache::SetCacheSkSL(settings.cache_sksl); TRACE_EVENT0("flutter", "Shell::CreateWithSnapshots");