Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit e9adfbde authored by Matt Buckley's avatar Matt Buckley
Browse files

Fix HWUI ShaderCache mIDHash eviction race 

This patch fixes the ShaderCache eviction race by ensuring the ID
remains in the cache outside of the background thread, during set().

This patch also makes set() a class method, and removes
getBlobCacheLocked() for simplicity as it no longer serves to
"ensure the blob cache is initialized" as initShaderDiskCache() handles
that role now.

Test: atest hwui_unit_tests:ShaderCacheTest
Bug: 290231463
Change-Id: Id69aff95a0cf3bed6b3b7dfc2bd61b2a8a2b555a
parent 61a737c6

libs/hwui/pipeline/skia/ShaderCache.cpp

+12 −20
Original line number Diff line number Diff line
@@ -88,6 +88,9 @@ void ShaderCache::initShaderDiskCache(const void* identity, ssize_t size) { 
        mBlobCache.reset(new FileBlobCache(maxKeySize, maxValueSize, maxTotalSize, mFilename));

        validateCache(identity, size);

        mInitialized = true;

        if (identity != nullptr && size > 0 && mIDHash.size()) {

            set(&sIDKey, sizeof(sIDKey), mIDHash.data(), mIDHash.size());

        }

    }

}
@@ -96,11 +99,6 @@ void ShaderCache::setFilename(const char* filename) { 
    mFilename = filename;

}



BlobCache* ShaderCache::getBlobCacheLocked() {

    LOG_ALWAYS_FATAL_IF(!mInitialized, "ShaderCache has not been initialized");

    return mBlobCache.get();

}



sk_sp<SkData> ShaderCache::load(const SkData& key) {

    ATRACE_NAME("ShaderCache::load");

    size_t keySize = key.size();
@@ -115,8 +113,7 @@ sk_sp<SkData> ShaderCache::load(const SkData& key) { 
    if (!valueBuffer) {

        return nullptr;

    }

    BlobCache* bc = getBlobCacheLocked();

    size_t valueSize = bc->get(key.data(), keySize, valueBuffer, mObservedBlobValueSize);

    size_t valueSize = mBlobCache->get(key.data(), keySize, valueBuffer, mObservedBlobValueSize);

    int maxTries = 3;

    while (valueSize > mObservedBlobValueSize && maxTries > 0) {

        mObservedBlobValueSize = std::min(valueSize, maxValueSize);
@@ -126,7 +123,7 @@ sk_sp<SkData> ShaderCache::load(const SkData& key) { 
            return nullptr;

        }

        valueBuffer = newValueBuffer;

        valueSize = bc->get(key.data(), keySize, valueBuffer, mObservedBlobValueSize);

        valueSize = mBlobCache->get(key.data(), keySize, valueBuffer, mObservedBlobValueSize);

        maxTries--;

    }

    if (!valueSize) {
@@ -143,16 +140,17 @@ sk_sp<SkData> ShaderCache::load(const SkData& key) { 
    return SkData::MakeFromMalloc(valueBuffer, valueSize);

}



namespace {

// Helper for BlobCache::set to trace the result.

void set(BlobCache* cache, const void* key, size_t keySize, const void* value, size_t valueSize) {

    switch (cache->set(key, keySize, value, valueSize)) {

void ShaderCache::set(const void* key, size_t keySize, const void* value, size_t valueSize) {

    switch (mBlobCache->set(key, keySize, value, valueSize)) {

        case BlobCache::InsertResult::kInserted:

            // This is what we expect/hope. It means the cache is large enough.

            return;

        case BlobCache::InsertResult::kDidClean: {

            ATRACE_FORMAT("ShaderCache: evicted an entry to fit {key: %lu value %lu}!", keySize,

                          valueSize);

            if (mIDHash.size()) {

                set(&sIDKey, sizeof(sIDKey), mIDHash.data(), mIDHash.size());

            }

            return;

        }

        case BlobCache::InsertResult::kNotEnoughSpace: {
@@ -172,15 +170,10 @@ void set(BlobCache* cache, const void* key, size_t keySize, const void* value, s 
        }

    }

}

}  // namespace



void ShaderCache::saveToDiskLocked() {

    ATRACE_NAME("ShaderCache::saveToDiskLocked");

    if (mInitialized && mBlobCache) {

        if (mIDHash.size()) {

            auto key = sIDKey;

            set(mBlobCache.get(), &key, sizeof(key), mIDHash.data(), mIDHash.size());

        }

        // The most straightforward way to make ownership shared

        mMutex.unlock();

        mMutex.lock_shared();
@@ -209,11 +202,10 @@ void ShaderCache::store(const SkData& key, const SkData& data, const SkString& / 


    const void* value = data.data();



    BlobCache* bc = getBlobCacheLocked();

    if (mInStoreVkPipelineInProgress) {

        if (mOldPipelineCacheSize == -1) {

            // Record the initial pipeline cache size stored in the file.

            mOldPipelineCacheSize = bc->get(key.data(), keySize, nullptr, 0);

            mOldPipelineCacheSize = mBlobCache->get(key.data(), keySize, nullptr, 0);

        }

        if (mNewPipelineCacheSize != -1 && mNewPipelineCacheSize == valueSize) {

            // There has not been change in pipeline cache size. Stop trying to save.
@@ -228,7 +220,7 @@ void ShaderCache::store(const SkData& key, const SkData& data, const SkString& / 
        mNewPipelineCacheSize = -1;

        mTryToStorePipelineCache = true;

    }

    set(bc, key.data(), keySize, value, valueSize);

    set(key.data(), keySize, value, valueSize);



    if (!mSavePending && mDeferredSaveDelayMs > 0) {

        mSavePending = true;

libs/hwui/pipeline/skia/ShaderCache.h

+9 −13
Original line number Diff line number Diff line
@@ -95,20 +95,18 @@ private: 
    ShaderCache(const ShaderCache&) = delete;

    void operator=(const ShaderCache&) = delete;



    /**

     * "getBlobCacheLocked" returns the BlobCache object being used to store the

     * key/value blob pairs.  If the BlobCache object has not yet been created,

     * this will do so, loading the serialized cache contents from disk if

     * possible.

     */

    BlobCache* getBlobCacheLocked() REQUIRES(mMutex);



    /**

     * "validateCache" updates the cache to match the given identity.  If the

     * cache currently has the wrong identity, all entries in the cache are cleared.

     */

    bool validateCache(const void* identity, ssize_t size) REQUIRES(mMutex);



    /**

     * Helper for BlobCache::set to trace the result and ensure the identity hash

     * does not get evicted.

     */

    void set(const void* key, size_t keySize, const void* value, size_t valueSize) REQUIRES(mMutex);



    /**

     * "saveToDiskLocked" attempts to save the current contents of the cache to

     * disk. If the identity hash exists, we will insert the identity hash into
@@ -127,11 +125,9 @@ private: 
    bool mInitialized GUARDED_BY(mMutex) = false;



    /**

     * "mBlobCache" is the cache in which the key/value blob pairs are stored.  It

     * is initially NULL, and will be initialized by getBlobCacheLocked the

     * first time it's needed.

     * The blob cache contains the Android build number. We treat version mismatches as an empty

     * cache (logic implemented in BlobCache::unflatten).

     * "mBlobCache" is the cache in which the key/value blob pairs are stored.

     * The blob cache contains the Android build number. We treat version mismatches

     * as an empty cache (logic implemented in BlobCache::unflatten).

     */

    std::unique_ptr<FileBlobCache> mBlobCache GUARDED_BY(mMutex);