EGL Multifile Blobcache: Clean up key reuse

This CL contains multiple fixes that allow key reuse.

* set() now checks whether an entry is already present, and if so
  removes it from hotcache and tracking before adding them.
* trackEntry and removeEntry now update the total cache size.
* applyLRU now correctly removes entries from all tracking.

Additional tests:
* SameKeyDifferentValues
* SameKeyLargeValues

Based on work by: Igor Nazarov <i.nazarov@samsung.com>

Test: libEGL_test, EGL_test, ANGLE trace tests, apps
Bug: b/351867582, b/380483358
Flag: com.android.graphics.egl.flags.multifile_blobcache_advanced_usage
Change-Id: I66fe9cde18e468e541a80296c80f2234ac61acb0

Author: cnorthrop

opengl/libs/EGL/MultifileBlobCache.cpp

@@ -246,12 +246,9 @@ MultifileBlobCache::MultifileBlobCache(size_t maxKeySize, size_t maxValueSize, s
 
                 ALOGV("INIT: Entry %u is good, tracking it now.", entryHash);
 
-                // Track details for rapid lookup later
+                // Track details for rapid lookup later and update total size
                 trackEntry(entryHash, header.valueSize, fileSize, st.st_atime);
 
-                // Track the total size
-                increaseTotalCacheSize(fileSize);
-
                 // Preload the entry for fast retrieval
                 if ((mHotCacheSize + fileSize) < mHotCacheLimit) {
                     ALOGV("INIT: Populating hot cache with fd = %i, cacheEntry = %p for "
@@ -326,6 +323,15 @@ void MultifileBlobCache::set(const void* key, EGLsizeiANDROID keySize, const voi
     // Generate a hash of the key and use it to track this entry
     uint32_t entryHash = android::JenkinsHashMixBytes(0, static_cast<const uint8_t*>(key), keySize);
 
+    // See if we already have this file
+    if (flags::multifile_blobcache_advanced_usage() && contains(entryHash)) {
+        // Remove previous entry from hot cache
+        removeFromHotCache(entryHash);
+
+        // Remove previous entry and update the overall cache size
+        removeEntry(entryHash);
+    }
+
     size_t fileSize = sizeof(MultifileHeader) + keySize + valueSize;
 
     // If we're going to be over the cache limit, kick off a trim to clear space
@@ -350,12 +356,9 @@ void MultifileBlobCache::set(const void* key, EGLsizeiANDROID keySize, const voi
 
     std::string fullPath = mMultifileDirName + "/" + std::to_string(entryHash);
 
-    // Track the size and access time for quick recall
+    // Track the size and access time for quick recall and update the overall cache size
     trackEntry(entryHash, valueSize, fileSize, time(0));
 
-    // Update the overall cache size
-    increaseTotalCacheSize(fileSize);
-
     // Keep the entry in hot cache for quick retrieval
     ALOGV("SET: Adding %u to hot cache.", entryHash);
 
@@ -638,6 +641,27 @@ void MultifileBlobCache::trackEntry(uint32_t entryHash, EGLsizeiANDROID valueSiz
                                     time_t accessTime) {
     mEntries.insert(entryHash);
     mEntryStats[entryHash] = {valueSize, fileSize, accessTime};
+
+    increaseTotalCacheSize(fileSize);
+}
+
+bool MultifileBlobCache::removeEntry(uint32_t entryHash) {
+    auto entryIter = mEntries.find(entryHash);
+    if (entryIter == mEntries.end()) {
+        return false;
+    }
+
+    auto entryStatsIter = mEntryStats.find(entryHash);
+    if (entryStatsIter == mEntryStats.end()) {
+        ALOGE("Failed to remove entryHash (%u) from mEntryStats", entryHash);
+        return false;
+    }
+    decreaseTotalCacheSize(entryStatsIter->second.fileSize);
+
+    mEntryStats.erase(entryStatsIter);
+    mEntries.erase(entryIter);
+
+    return true;
 }
 
 bool MultifileBlobCache::contains(uint32_t hashEntry) const {
@@ -728,13 +752,10 @@ bool MultifileBlobCache::applyLRU(size_t cacheSizeLimit, size_t cacheEntryLimit)
     // Walk through our map of sorted last access times and remove files until under the limit
     for (auto cacheEntryIter = mEntryStats.begin(); cacheEntryIter != mEntryStats.end();) {
         uint32_t entryHash = cacheEntryIter->first;
+        const MultifileEntryStats& entryStats = cacheEntryIter->second;
 
         ALOGV("LRU: Removing entryHash %u", entryHash);
 
-        // Track the overall size
-        MultifileEntryStats entryStats = getEntryStats(entryHash);
-        decreaseTotalCacheSize(entryStats.fileSize);
-
         // Remove it from hot cache if present
         removeFromHotCache(entryHash);
 
@@ -748,10 +769,9 @@ bool MultifileBlobCache::applyLRU(size_t cacheSizeLimit, size_t cacheEntryLimit)
         // Increment the iterator before clearing the entry
         cacheEntryIter++;
 
-        // Delete the entry from our tracking
-        size_t count = mEntryStats.erase(entryHash);
-        if (count != 1) {
-            ALOGE("LRU: Failed to remove entryHash (%u) from mEntryStats", entryHash);
+        // Delete the entry from our tracking and update the overall cache size
+        if (!removeEntry(entryHash)) {
+            ALOGE("LRU: Failed to remove entryHash %u", entryHash);
             return false;
         }
 

opengl/libs/EGL/MultifileBlobCache_test.cpp

@@ -509,4 +509,102 @@ TEST_F(MultifileBlobCacheTest, MismatchedBuildIdClears) {
     ASSERT_EQ(getCacheEntries().size(), 0);
 }
 
+// Ensure cache is correct when a key is reused
+TEST_F(MultifileBlobCacheTest, SameKeyDifferentValues) {
+    if (!flags::multifile_blobcache_advanced_usage()) {
+        GTEST_SKIP() << "Skipping test that requires multifile_blobcache_advanced_usage flag";
+    }
+
+    unsigned char buf[4] = {0xee, 0xee, 0xee, 0xee};
+
+    size_t startingSize = mMBC->getTotalSize();
+
+    // New cache should be empty
+    ASSERT_EQ(startingSize, 0);
+
+    // Set an initial value
+    mMBC->set("ab", 2, "cdef", 4);
+
+    // Grab the new size
+    size_t firstSize = mMBC->getTotalSize();
+
+    // Ensure the size went up
+    // Note: Checking for an exact size is challenging, as the
+    // file size can differ between platforms.
+    ASSERT_GT(firstSize, startingSize);
+
+    // Verify the cache is correct
+    ASSERT_EQ(size_t(4), mMBC->get("ab", 2, buf, 4));
+    ASSERT_EQ('c', buf[0]);
+    ASSERT_EQ('d', buf[1]);
+    ASSERT_EQ('e', buf[2]);
+    ASSERT_EQ('f', buf[3]);
+
+    // Now reuse the key with a smaller value
+    mMBC->set("ab", 2, "gh", 2);
+
+    // Grab the new size
+    size_t secondSize = mMBC->getTotalSize();
+
+    // Ensure it decreased in size
+    ASSERT_LT(secondSize, firstSize);
+
+    // Verify the cache is correct
+    ASSERT_EQ(size_t(2), mMBC->get("ab", 2, buf, 2));
+    ASSERT_EQ('g', buf[0]);
+    ASSERT_EQ('h', buf[1]);
+
+    // Now put back the original value
+    mMBC->set("ab", 2, "cdef", 4);
+
+    // And we should get back a stable size
+    size_t finalSize = mMBC->getTotalSize();
+    ASSERT_EQ(firstSize, finalSize);
+}
+
+// Ensure cache is correct when a key is reused with large value size
+TEST_F(MultifileBlobCacheTest, SameKeyLargeValues) {
+    if (!flags::multifile_blobcache_advanced_usage()) {
+        GTEST_SKIP() << "Skipping test that requires multifile_blobcache_advanced_usage flag";
+    }
+
+    // Create the cache with larger limits to stress test reuse
+    constexpr uint32_t kLocalMaxKeySize = 1 * 1024 * 1024;
+    constexpr uint32_t kLocalMaxValueSize = 4 * 1024 * 1024;
+    constexpr uint32_t kLocalMaxTotalSize = 32 * 1024 * 1024;
+    mMBC.reset(new MultifileBlobCache(kLocalMaxKeySize, kLocalMaxValueSize, kLocalMaxTotalSize,
+                                      kMaxTotalEntries, &mTempFile->path[0]));
+
+    constexpr uint32_t kLargeValueCount = 8;
+    constexpr uint32_t kLargeValueSize = 64 * 1024;
+
+    // Create a several really large values
+    unsigned char largeValue[kLargeValueCount][kLargeValueSize];
+    for (int i = 0; i < kLargeValueCount; i++) {
+        for (int j = 0; j < kLargeValueSize; j++) {
+            // Fill the value with the index for uniqueness
+            largeValue[i][j] = i;
+        }
+    }
+
+    size_t startingSize = mMBC->getTotalSize();
+
+    // New cache should be empty
+    ASSERT_EQ(startingSize, 0);
+
+    // Cycle through the values and set them all in sequence
+    for (int i = 0; i < kLargeValueCount; i++) {
+        mMBC->set("abcd", 4, largeValue[i], kLargeValueSize);
+    }
+
+    // Ensure we get the last one back
+    unsigned char outBuf[kLargeValueSize];
+    mMBC->get("abcd", 4, outBuf, kLargeValueSize);
+
+    for (int i = 0; i < kLargeValueSize; i++) {
+        // Buffer should contain highest index value
+        ASSERT_EQ(kLargeValueCount - 1, outBuf[i]);
+    }
+}
+
 } // namespace android