From 9036882d49841883a673a401f932845ccf6361bb Mon Sep 17 00:00:00 2001 From: Yurii Zubrytskyi Date: Tue, 17 Dec 2024 19:21:16 -0800 Subject: [res] Optimize isUpToDate() for ApkAssets This is the most called function in the whole Resources codebase, and it often takes a millisecond+ to just check if the underlying file still exists. This CL optimizes several aspects of the function - knowing that all /data/ paths are writable, return early from the isReadonlyFilesystem() without a syscall - use the same getFileModDate() function for all native classes instead of the custom stat-based code - skip the modification time getter for all readonly filesystems when we know that the idmap, overlay and target can't change. - add some default-disabled logging code to print timings for easier performance measurement Bug: 319137634 Test: build, boot and atest libandroidfw_tests Flag: EXEMPT performance-critical code that can't have two versions Change-Id: I0f801bef386f202eda775cca9657e2a0b6b55c95 --- libs/androidfw/AssetsProvider.cpp | 65 ++++++++++++++++----------------------- 1 file changed, 26 insertions(+), 39 deletions(-) (limited to 'libs/androidfw/AssetsProvider.cpp') diff --git a/libs/androidfw/AssetsProvider.cpp b/libs/androidfw/AssetsProvider.cpp index 2d3c06506a1f..59c6aba7b8b0 100644 --- a/libs/androidfw/AssetsProvider.cpp +++ b/libs/androidfw/AssetsProvider.cpp @@ -86,11 +86,9 @@ void ZipAssetsProvider::ZipCloser::operator()(ZipArchive* a) const { } ZipAssetsProvider::ZipAssetsProvider(ZipArchiveHandle handle, PathOrDebugName&& path, - package_property_t flags, time_t last_mod_time) - : zip_handle_(handle), - name_(std::move(path)), - flags_(flags), - last_mod_time_(last_mod_time) {} + package_property_t flags, ModDate last_mod_time) + : zip_handle_(handle), name_(std::move(path)), flags_(flags), last_mod_time_(last_mod_time) { +} std::unique_ptr ZipAssetsProvider::Create(std::string path, package_property_t flags, @@ -104,10 +102,10 @@ std::unique_ptr ZipAssetsProvider::Create(std::string path, return {}; } - struct stat sb{.st_mtime = -1}; + ModDate mod_date = kInvalidModDate; // Skip all up-to-date checks if the file won't ever change. - if (!isReadonlyFilesystem(path.c_str())) { - if ((released_fd < 0 ? stat(path.c_str(), &sb) : fstat(released_fd, &sb)) < 0) { + if (isKnownWritablePath(path.c_str()) || !isReadonlyFilesystem(GetFileDescriptor(handle))) { + if (mod_date = getFileModDate(GetFileDescriptor(handle)); mod_date == kInvalidModDate) { // Stat requires execute permissions on all directories path to the file. If the process does // not have execute permissions on this file, allow the zip to be opened but IsUpToDate() will // always have to return true. @@ -116,7 +114,7 @@ std::unique_ptr ZipAssetsProvider::Create(std::string path, } return std::unique_ptr( - new ZipAssetsProvider(handle, PathOrDebugName::Path(std::move(path)), flags, sb.st_mtime)); + new ZipAssetsProvider(handle, PathOrDebugName::Path(std::move(path)), flags, mod_date)); } std::unique_ptr ZipAssetsProvider::Create(base::unique_fd fd, @@ -137,10 +135,10 @@ std::unique_ptr ZipAssetsProvider::Create(base::unique_fd fd, return {}; } - struct stat sb{.st_mtime = -1}; + ModDate mod_date = kInvalidModDate; // Skip all up-to-date checks if the file won't ever change. if (!isReadonlyFilesystem(released_fd)) { - if (fstat(released_fd, &sb) < 0) { + if (mod_date = getFileModDate(released_fd); mod_date == kInvalidModDate) { // Stat requires execute permissions on all directories path to the file. If the process does // not have execute permissions on this file, allow the zip to be opened but IsUpToDate() will // always have to return true. @@ -150,7 +148,7 @@ std::unique_ptr ZipAssetsProvider::Create(base::unique_fd fd, } return std::unique_ptr(new ZipAssetsProvider( - handle, PathOrDebugName::DebugName(std::move(friendly_name)), flags, sb.st_mtime)); + handle, PathOrDebugName::DebugName(std::move(friendly_name)), flags, mod_date)); } std::unique_ptr ZipAssetsProvider::OpenInternal(const std::string& path, @@ -282,21 +280,16 @@ const std::string& ZipAssetsProvider::GetDebugName() const { return name_.GetDebugName(); } -bool ZipAssetsProvider::IsUpToDate() const { - if (last_mod_time_ == -1) { - return true; - } - struct stat sb{}; - if (fstat(GetFileDescriptor(zip_handle_.get()), &sb) < 0) { - // If fstat fails on the zip archive, return true so the zip archive the resource system does - // attempt to refresh the ApkAsset. - return true; +UpToDate ZipAssetsProvider::IsUpToDate() const { + if (last_mod_time_ == kInvalidModDate) { + return UpToDate::Always; } - return last_mod_time_ == sb.st_mtime; + return fromBool(last_mod_time_ == getFileModDate(GetFileDescriptor(zip_handle_.get()))); } -DirectoryAssetsProvider::DirectoryAssetsProvider(std::string&& path, time_t last_mod_time) - : dir_(std::move(path)), last_mod_time_(last_mod_time) {} +DirectoryAssetsProvider::DirectoryAssetsProvider(std::string&& path, ModDate last_mod_time) + : dir_(std::move(path)), last_mod_time_(last_mod_time) { +} std::unique_ptr DirectoryAssetsProvider::Create(std::string path) { struct stat sb; @@ -317,7 +310,7 @@ std::unique_ptr DirectoryAssetsProvider::Create(std::st const bool isReadonly = isReadonlyFilesystem(path.c_str()); return std::unique_ptr( - new DirectoryAssetsProvider(std::move(path), isReadonly ? -1 : sb.st_mtime)); + new DirectoryAssetsProvider(std::move(path), isReadonly ? kInvalidModDate : getModDate(sb))); } std::unique_ptr DirectoryAssetsProvider::OpenInternal(const std::string& path, @@ -346,17 +339,11 @@ const std::string& DirectoryAssetsProvider::GetDebugName() const { return dir_; } -bool DirectoryAssetsProvider::IsUpToDate() const { - if (last_mod_time_ == -1) { - return true; +UpToDate DirectoryAssetsProvider::IsUpToDate() const { + if (last_mod_time_ == kInvalidModDate) { + return UpToDate::Always; } - struct stat sb; - if (stat(dir_.c_str(), &sb) < 0) { - // If stat fails on the zip archive, return true so the zip archive the resource system does - // attempt to refresh the ApkAsset. - return true; - } - return last_mod_time_ == sb.st_mtime; + return fromBool(last_mod_time_ == getFileModDate(dir_.c_str())); } MultiAssetsProvider::MultiAssetsProvider(std::unique_ptr&& primary, @@ -397,8 +384,8 @@ const std::string& MultiAssetsProvider::GetDebugName() const { return debug_name_; } -bool MultiAssetsProvider::IsUpToDate() const { - return primary_->IsUpToDate() && secondary_->IsUpToDate(); +UpToDate MultiAssetsProvider::IsUpToDate() const { + return combine(primary_->IsUpToDate(), [this] { return secondary_->IsUpToDate(); }); } EmptyAssetsProvider::EmptyAssetsProvider(std::optional&& path) : @@ -442,8 +429,8 @@ const std::string& EmptyAssetsProvider::GetDebugName() const { return kEmpty; } -bool EmptyAssetsProvider::IsUpToDate() const { - return true; +UpToDate EmptyAssetsProvider::IsUpToDate() const { + return UpToDate::Always; } } // namespace android -- cgit v1.2.3-59-g8ed1b From 2f271fe46646971c66beb6f0ea906a2c771b6a1c Mon Sep 17 00:00:00 2001 From: Yurii Zubrytskyi Date: Thu, 19 Dec 2024 17:53:13 -0800 Subject: [res] Don't create extra asset provider when not needed MultiAssetsProvider is useful to combine a ResourcesLoader with a file-based provider, but most of the time it's used without any loaders, and ends up with an EmptyAssetsProvider object that just wastes cycles and RAM. This CL optimizes that out, only creating the extra layer when we have a real loader, and falling back to the only remaining provider otherwise Bug: 319137634 Test: build + boot + manual + atest libandroidfw_tests Flag: EXEMPT optimization Change-Id: Ibc5ac60adc5e008b70a62481a747758c89083ff9 --- core/jni/android_content_res_ApkAssets.cpp | 56 +++++++++++++---------- libs/androidfw/AssetsProvider.cpp | 17 ++++--- libs/androidfw/include/androidfw/AssetsProvider.h | 2 +- 3 files changed, 43 insertions(+), 32 deletions(-) (limited to 'libs/androidfw/AssetsProvider.cpp') diff --git a/core/jni/android_content_res_ApkAssets.cpp b/core/jni/android_content_res_ApkAssets.cpp index 0da9b188ac5a..e6364a96bd9f 100644 --- a/core/jni/android_content_res_ApkAssets.cpp +++ b/core/jni/android_content_res_ApkAssets.cpp @@ -111,9 +111,8 @@ static void DeleteGuardedApkAssets(Guarded& apk_ass class LoaderAssetsProvider : public AssetsProvider { public: static std::unique_ptr Create(JNIEnv* env, jobject assets_provider) { - return (!assets_provider) ? EmptyAssetsProvider::Create() - : std::unique_ptr(new LoaderAssetsProvider( - env, assets_provider)); + return std::unique_ptr{ + assets_provider ? new LoaderAssetsProvider(env, assets_provider) : nullptr}; } bool ForEachFile(const std::string& /* root_path */, @@ -212,7 +211,7 @@ class LoaderAssetsProvider : public AssetsProvider { auto string_result = static_cast(env->CallObjectMethod( assets_provider_, gAssetsProviderOffsets.toString)); ScopedUtfChars str(env, string_result); - debug_name_ = std::string(str.c_str(), str.size()); + debug_name_ = std::string(str.c_str()); } // The global reference to the AssetsProvider @@ -243,10 +242,10 @@ static jlong NativeLoad(JNIEnv* env, jclass /*clazz*/, const format_type_t forma apk_assets = ApkAssets::LoadOverlay(path.c_str(), property_flags); break; case FORMAT_ARSC: - apk_assets = ApkAssets::LoadTable(AssetsProvider::CreateAssetFromFile(path.c_str()), - std::move(loader_assets), - property_flags); - break; + apk_assets = ApkAssets::LoadTable(AssetsProvider::CreateAssetFromFile(path.c_str()), + MultiAssetsProvider::Create(std::move(loader_assets)), + property_flags); + break; case FORMAT_DIRECTORY: { auto assets = MultiAssetsProvider::Create(std::move(loader_assets), DirectoryAssetsProvider::Create(path.c_str())); @@ -316,10 +315,11 @@ static jlong NativeLoadFromFd(JNIEnv* env, jclass /*clazz*/, const format_type_t break; } case FORMAT_ARSC: - apk_assets = ApkAssets::LoadTable( - AssetsProvider::CreateAssetFromFd(std::move(dup_fd), nullptr /* path */), - std::move(loader_assets), property_flags); - break; + apk_assets = ApkAssets::LoadTable(AssetsProvider::CreateAssetFromFd(std::move(dup_fd), + nullptr /* path */), + MultiAssetsProvider::Create(std::move(loader_assets)), + property_flags); + break; default: const std::string error_msg = base::StringPrintf("Unsupported format type %d", format); jniThrowException(env, "java/lang/IllegalArgumentException", error_msg.c_str()); @@ -386,12 +386,15 @@ static jlong NativeLoadFromFdOffset(JNIEnv* env, jclass /*clazz*/, const format_ break; } case FORMAT_ARSC: - apk_assets = ApkAssets::LoadTable( - AssetsProvider::CreateAssetFromFd(std::move(dup_fd), nullptr /* path */, - static_cast(offset), - static_cast(length)), - std::move(loader_assets), property_flags); - break; + apk_assets = + ApkAssets::LoadTable(AssetsProvider::CreateAssetFromFd(std::move(dup_fd), + nullptr /* path */, + static_cast(offset), + static_cast( + length)), + MultiAssetsProvider::Create(std::move(loader_assets)), + property_flags); + break; default: const std::string error_msg = base::StringPrintf("Unsupported format type %d", format); jniThrowException(env, "java/lang/IllegalArgumentException", error_msg.c_str()); @@ -408,13 +411,16 @@ static jlong NativeLoadFromFdOffset(JNIEnv* env, jclass /*clazz*/, const format_ } static jlong NativeLoadEmpty(JNIEnv* env, jclass /*clazz*/, jint flags, jobject assets_provider) { - auto apk_assets = ApkAssets::Load(LoaderAssetsProvider::Create(env, assets_provider), flags); - if (apk_assets == nullptr) { - const std::string error_msg = - base::StringPrintf("Failed to load empty assets with provider %p", (void*)assets_provider); - jniThrowException(env, "java/io/IOException", error_msg.c_str()); - return 0; - } + auto apk_assets = ApkAssets::Load(MultiAssetsProvider::Create( + LoaderAssetsProvider::Create(env, assets_provider)), + flags); + if (apk_assets == nullptr) { + const std::string error_msg = + base::StringPrintf("Failed to load empty assets with provider %p", + (void*)assets_provider); + jniThrowException(env, "java/io/IOException", error_msg.c_str()); + return 0; + } return CreateGuardedApkAssets(std::move(apk_assets)); } diff --git a/libs/androidfw/AssetsProvider.cpp b/libs/androidfw/AssetsProvider.cpp index 59c6aba7b8b0..11b12eb030a6 100644 --- a/libs/androidfw/AssetsProvider.cpp +++ b/libs/androidfw/AssetsProvider.cpp @@ -24,9 +24,8 @@ #include namespace android { -namespace { -constexpr const char* kEmptyDebugString = ""; -} // namespace + +static constexpr std::string_view kEmptyDebugString = ""; std::unique_ptr AssetsProvider::Open(const std::string& path, Asset::AccessMode mode, bool* file_exists) const { @@ -356,8 +355,14 @@ MultiAssetsProvider::MultiAssetsProvider(std::unique_ptr&& prima std::unique_ptr MultiAssetsProvider::Create( std::unique_ptr&& primary, std::unique_ptr&& secondary) { - if (primary == nullptr || secondary == nullptr) { - return nullptr; + if (primary == nullptr && secondary == nullptr) { + return EmptyAssetsProvider::Create(); + } + if (!primary) { + return secondary; + } + if (!secondary) { + return primary; } return std::unique_ptr(new MultiAssetsProvider(std::move(primary), std::move(secondary))); @@ -425,7 +430,7 @@ const std::string& EmptyAssetsProvider::GetDebugName() const { if (path_.has_value()) { return *path_; } - const static std::string kEmpty = kEmptyDebugString; + constexpr static std::string kEmpty{kEmptyDebugString}; return kEmpty; } diff --git a/libs/androidfw/include/androidfw/AssetsProvider.h b/libs/androidfw/include/androidfw/AssetsProvider.h index 46d7a57ab763..e3b3ae41f7f4 100644 --- a/libs/androidfw/include/androidfw/AssetsProvider.h +++ b/libs/androidfw/include/androidfw/AssetsProvider.h @@ -164,7 +164,7 @@ struct DirectoryAssetsProvider : public AssetsProvider { // `secondary` asset provider if the asset cannot be found in the `primary`. struct MultiAssetsProvider : public AssetsProvider { static std::unique_ptr Create(std::unique_ptr&& primary, - std::unique_ptr&& secondary); + std::unique_ptr&& secondary = {}); bool ForEachFile(const std::string& root_path, base::function_ref f) const override; -- cgit v1.2.3-59-g8ed1b