Reland "[native] Update caching to use the new callbacks and spanified APIs." This is a reland of commit 91cecb6c616bb39f71b0ba3f8b563aa621189ba3 Original change's description: > [native] Update caching to use the new callbacks and spanified APIs. > > Bug: 503801946 > Change-Id: Ibee701057ba67537e4594116f7eafb7271407e1a > Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/314856 > Commit-Queue: Loko Kung <lokokung@google.com> > Reviewed-by: Kyle Charbonneau <kylechar@google.com> > Reviewed-by: Kai Ninomiya <kainino@chromium.org> Bug: 503801946 Change-Id: I5458b84bbdc7e59cf9b4119a77ad81703ed12902 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/323195 Reviewed-by: Kai Ninomiya <kainino@chromium.org> Commit-Queue: Kai Ninomiya <kainino@chromium.org> Auto-Submit: Loko Kung <lokokung@google.com>
diff --git a/src/dawn/dawn.json b/src/dawn/dawn.json index 1312331..c4c903b 100644 --- a/src/dawn/dawn.json +++ b/src/dawn/dawn.json
@@ -308,9 +308,6 @@ "chain roots": ["device descriptor"], "members": [ {"name": "isolation key", "type": "string view"}, - {"name": "load data function", "type": "dawn load cache data function", "default": "nullptr"}, - {"name": "store data function", "type": "dawn store cache data function", "default": "nullptr"}, - {"name": "function userdata", "type": "void *", "default": "nullptr"}, {"name": "dawn load cache data callback info", "type": "dawn load cache data callback info"}, {"name": "dawn store cache data callback info", "type": "dawn store cache data callback info"} ]
diff --git a/src/dawn/native/BlobCache.cpp b/src/dawn/native/BlobCache.cpp index f3d0aa7..c2ad2ca 100644 --- a/src/dawn/native/BlobCache.cpp +++ b/src/dawn/native/BlobCache.cpp
@@ -98,9 +98,6 @@ BlobCache::BlobCache(const dawn::native::DawnCacheDeviceDescriptor& desc, bool enableHashValidation) : mHashValidation(enableHashValidation), - mLoadFunction(desc.loadDataFunction), - mStoreFunction(desc.storeDataFunction), - mFunctionUserdata(desc.functionUserdata), mLoadCallbackInfo(desc.dawnLoadCacheDataCallbackInfo), mStoreCallbackInfo(desc.dawnStoreCacheDataCallbackInfo) {} @@ -132,19 +129,14 @@ // Make sure we early out if we are not using storing functionality. Otherwise, computing the // hash may add unnecessary overhead. - if (mStoreFunction == nullptr && mStoreCallbackInfo.callback == nullptr) { + if (mStoreCallbackInfo.callback == nullptr) { return; } auto store = [&](std::span<const std::byte> actualValue) { - if (mStoreCallbackInfo.callback != nullptr) { - mStoreCallbackInfo.callback( - cacheKey.size(), reinterpret_cast<const uint8_t*>(cacheKey.data()), - actualValue.size(), reinterpret_cast<const uint8_t*>(actualValue.data()), - mStoreCallbackInfo.userdata1, mStoreCallbackInfo.userdata2); - } else if (mStoreFunction != nullptr) { - mStoreFunction(cacheKey.data(), cacheKey.size(), actualValue.data(), actualValue.size(), - mFunctionUserdata); - } + mStoreCallbackInfo.callback( + cacheKey.size(), reinterpret_cast<const uint8_t*>(cacheKey.data()), actualValue.size(), + reinterpret_cast<const uint8_t*>(actualValue.data()), mStoreCallbackInfo.userdata1, + mStoreCallbackInfo.userdata2); }; // Call the actual store function for actual stored bytes. @@ -159,17 +151,14 @@ ResultOrError<Blob> BlobCache::LoadInternal(const CacheKey& cacheKey) { DAWN_ASSERT(ValidateCacheKey(cacheKey)); + if (mLoadCallbackInfo.callback == nullptr) { + return Blob(); + } auto load = [&](std::span<std::byte> value) -> size_t { - if (mLoadCallbackInfo.callback != nullptr) { - return mLoadCallbackInfo.callback( - cacheKey.size(), reinterpret_cast<const uint8_t*>(cacheKey.data()), value.size(), - reinterpret_cast<uint8_t*>(value.data()), mLoadCallbackInfo.userdata1, - mLoadCallbackInfo.userdata2); - } else if (mLoadFunction != nullptr) { - return mLoadFunction(cacheKey.data(), cacheKey.size(), value.data(), value.size(), - mFunctionUserdata); - } - return 0; + return mLoadCallbackInfo.callback(cacheKey.size(), + reinterpret_cast<const uint8_t*>(cacheKey.data()), + value.size(), reinterpret_cast<uint8_t*>(value.data()), + mLoadCallbackInfo.userdata1, mLoadCallbackInfo.userdata2); }; const size_t expectedSize = load({}); @@ -178,7 +167,7 @@ // Load bytes from cache. Blob result = Blob::Create(expectedSize); const size_t actualSize = load(result.Data()); - // TODO(crbug.com/469351711): If `mLoadFunction` (or the new callback) returns a different + // TODO(crbug.com/469351711): If `mLoadCallbackInfo`'s callback returns a different // size on the second call (due to external cache eviction, I/O errors, or timeouts), treat // it as a cache miss. The blob cache API should be updated to a single `mLoadFunction` call // in the future.
diff --git a/src/dawn/native/BlobCache.h b/src/dawn/native/BlobCache.h index 2884a76..a441ab7 100644 --- a/src/dawn/native/BlobCache.h +++ b/src/dawn/native/BlobCache.h
@@ -33,7 +33,6 @@ #include <span> #include <vector> -#include "partition_alloc/pointers/raw_ptr_exclusion.h" #include "src/dawn/common/Sha3.h" #include "src/dawn/native/Blob.h" #include "src/dawn/native/CacheResult.h" @@ -94,10 +93,6 @@ bool ValidateCacheKey(const CacheKey& key); const bool mHashValidation; - // TODO(https://crbug.com/dawn/2365): Convert these members to `raw_ptr`. - RAW_PTR_EXCLUSION const WGPUDawnLoadCacheDataFunction mLoadFunction; - RAW_PTR_EXCLUSION const WGPUDawnStoreCacheDataFunction mStoreFunction; - RAW_PTR_EXCLUSION void* const mFunctionUserdata; const WGPUDawnLoadCacheDataCallbackInfo mLoadCallbackInfo; const WGPUDawnStoreCacheDataCallbackInfo mStoreCallbackInfo; };
diff --git a/src/dawn/native/Device.cpp b/src/dawn/native/Device.cpp index 4ce6806..bea01dd 100644 --- a/src/dawn/native/Device.cpp +++ b/src/dawn/native/Device.cpp
@@ -298,27 +298,33 @@ cacheDesc = *cacheDescIn; } - if (cacheDesc.loadDataFunction == nullptr && cacheDesc.storeDataFunction == nullptr && - cacheDesc.functionUserdata == nullptr && GetPlatform()->GetCachingInterface() != nullptr) { + if (cacheDesc.dawnLoadCacheDataCallbackInfo.callback == nullptr && + cacheDesc.dawnStoreCacheDataCallbackInfo.callback == nullptr && + GetPlatform()->GetCachingInterface() != nullptr) { // Populate cache functions and userdata from legacy cachingInterface. - cacheDesc.loadDataFunction = [](const void* key, size_t keySize, void* value, - size_t valueSize, void* userdata) { - auto* cachingInterface = static_cast<dawn::platform::CachingInterface*>(userdata); - return cachingInterface->LoadData(key, keySize, value, valueSize); - }; - cacheDesc.storeDataFunction = [](const void* key, size_t keySize, const void* value, - size_t valueSize, void* userdata) { - auto* cachingInterface = static_cast<dawn::platform::CachingInterface*>(userdata); - return cachingInterface->StoreData(key, keySize, value, valueSize); - }; - cacheDesc.functionUserdata = GetPlatform()->GetCachingInterface(); + ToCppAPI(&cacheDesc) + ->SetDawnLoadCacheDataCallback( + [](std::span<const std::byte> key, std::span<std::byte> value, + dawn::platform::CachingInterface* cachingInterface) -> size_t { + if (value.empty()) { + return cachingInterface->FindKey(key); + } + return cachingInterface->LoadData(key, value); + }, + GetPlatform()->GetCachingInterface()); + ToCppAPI(&cacheDesc) + ->SetDawnStoreCacheDataCallback( + [](std::span<const std::byte> key, std::span<const std::byte> value, + dawn::platform::CachingInterface* cachingInterface) { + cachingInterface->StoreData(key, value); + }, + GetPlatform()->GetCachingInterface()); } // Disable caching if the DisableBlobCache toggle is enabled. if (IsToggleEnabled(Toggle::DisableBlobCache)) { - cacheDesc.loadDataFunction = nullptr; - cacheDesc.storeDataFunction = nullptr; - cacheDesc.functionUserdata = nullptr; + cacheDesc.dawnLoadCacheDataCallbackInfo = {}; + cacheDesc.dawnStoreCacheDataCallbackInfo = {}; } mBlobCache =
diff --git a/src/dawn/tests/mocks/platform/CachingInterfaceMock.cpp b/src/dawn/tests/mocks/platform/CachingInterfaceMock.cpp index 4405988..1f91a66 100644 --- a/src/dawn/tests/mocks/platform/CachingInterfaceMock.cpp +++ b/src/dawn/tests/mocks/platform/CachingInterfaceMock.cpp
@@ -27,9 +27,16 @@ #include "src/dawn/tests/mocks/platform/CachingInterfaceMock.h" +#include <algorithm> +#include <utility> + +#include "src/utils/assert.h" #include "src/utils/compiler.h" CachingInterfaceMock::CachingInterfaceMock() { + ON_CALL(*this, FindKey).WillByDefault([this](auto&&... args) { + return FindKeyDefault(args...); + }); ON_CALL(*this, LoadData).WillByDefault([this](auto&&... args) { return LoadDataDefault(args...); }); @@ -58,41 +65,48 @@ return mCache.size(); } -size_t CachingInterfaceMock::LoadDataDefault(const void* key, - size_t keySize, - void* value, - size_t valueSize) { +size_t CachingInterfaceMock::FindKeyDefault(std::span<const std::byte> key) { std::scoped_lock lock(mMutex); if (!mEnabled) { return 0; } - const std::string keyStr(reinterpret_cast<const char*>(key), keySize); - auto entry = mCache.find(keyStr); + std::vector<std::byte> keyVec(key.begin(), key.end()); + auto entry = mCache.find(keyVec); if (entry == mCache.end()) { return 0; } - if (valueSize >= entry->second.size()) { - // Only consider a cache-hit on the memcpy, since peeks are implementation detail. - DAWN_UNSAFE_TODO(memcpy(value, entry->second.data(), entry->second.size())); - mHitCount++; - } return entry->second.size(); } -void CachingInterfaceMock::StoreDataDefault(const void* key, - size_t keySize, - const void* value, - size_t valueSize) { +size_t CachingInterfaceMock::LoadDataDefault(std::span<const std::byte> key, + std::span<std::byte> dest) { + std::scoped_lock lock(mMutex); + if (!mEnabled) { + return 0; + } + + std::vector<std::byte> keyVec(key.begin(), key.end()); + auto entry = mCache.find(keyVec); + if (entry == mCache.end()) { + return 0; + } + DAWN_CHECK(dest.size() >= entry->second.size()); + std::ranges::copy(entry->second, dest.begin()); + mHitCount++; + return entry->second.size(); +} + +void CachingInterfaceMock::StoreDataDefault(std::span<const std::byte> key, + std::span<const std::byte> src) { std::scoped_lock lock(mMutex); if (!mEnabled) { return; } - const std::string keyStr(reinterpret_cast<const char*>(key), keySize); - const uint8_t* it = reinterpret_cast<const uint8_t*>(value); - std::vector<uint8_t> entry(it, DAWN_UNSAFE_TODO(it + valueSize)); - mCache.insert_or_assign(keyStr, entry); + std::vector<std::byte> keyVec(key.begin(), key.end()); + std::vector<std::byte> entry(src.begin(), src.end()); + mCache.insert_or_assign(std::move(keyVec), std::move(entry)); } DawnCachingMockPlatform::DawnCachingMockPlatform(dawn::platform::CachingInterface* cachingInterface)
diff --git a/src/dawn/tests/mocks/platform/CachingInterfaceMock.h b/src/dawn/tests/mocks/platform/CachingInterfaceMock.h index afecfad..b92b1b1 100644 --- a/src/dawn/tests/mocks/platform/CachingInterfaceMock.h +++ b/src/dawn/tests/mocks/platform/CachingInterfaceMock.h
@@ -31,9 +31,9 @@ #include <dawn/platform/DawnPlatform.h> #include <gmock/gmock.h> +#include <map> #include <mutex> -#include <string> -#include <unordered_map> +#include <span> #include <vector> #include "partition_alloc/pointers/raw_ptr.h" @@ -84,17 +84,25 @@ // Returns the number of entries in the cache. size_t GetNumEntries() const; - MOCK_METHOD(size_t, LoadData, (const void*, size_t, void*, size_t), (override)); - MOCK_METHOD(void, StoreData, (const void*, size_t, const void*, size_t), (override)); + MOCK_METHOD(size_t, FindKey, (std::span<const std::byte> key), (override)); + MOCK_METHOD(size_t, + LoadData, + (std::span<const std::byte> key, std::span<std::byte> dest), + (override)); + MOCK_METHOD(void, + StoreData, + (std::span<const std::byte> key, std::span<const std::byte> src), + (override)); private: - size_t LoadDataDefault(const void* key, size_t keySize, void* value, size_t valueSize); - void StoreDataDefault(const void* key, size_t keySize, const void* value, size_t valueSize); + size_t FindKeyDefault(std::span<const std::byte> key); + size_t LoadDataDefault(std::span<const std::byte> key, std::span<std::byte> dest); + void StoreDataDefault(std::span<const std::byte> key, std::span<const std::byte> src); mutable std::mutex mMutex; bool mEnabled = true; size_t mHitCount = 0; - std::unordered_map<std::string, std::vector<uint8_t>> mCache; + std::map<std::vector<std::byte>, std::vector<std::byte>> mCache; }; // Dawn platform used for testing with a mock caching interface.
diff --git a/src/dawn/tests/unittests/native/CacheRequestTests.cpp b/src/dawn/tests/unittests/native/CacheRequestTests.cpp index 2d6ff21..bdc3124 100644 --- a/src/dawn/tests/unittests/native/CacheRequestTests.cpp +++ b/src/dawn/tests/unittests/native/CacheRequestTests.cpp
@@ -100,7 +100,7 @@ // static_assert the expected types for various return types from the cache hit handler and cache // miss handler. TEST_P(CacheRequestTests, CacheResultTypes) { - EXPECT_CALL(mMockCache, LoadData(_, _, nullptr, 0)).WillRepeatedly(Return(0)); + EXPECT_CALL(mMockCache, FindKey(_)).WillRepeatedly(Return(0)); // (int, ResultOrError<int>), should be ResultOrError<CacheResult<int>>. auto v1 = LoadOrRun( @@ -131,10 +131,11 @@ StreamIn(&expectedKey, GetDevice()->GetCacheKey(), "CacheRequestForTesting", req.a, req.b, req.c); - // Expect a call to LoadData with the expected key. - EXPECT_CALL(mMockCache, LoadData(_, expectedKey.size(), nullptr, 0)) - .WillOnce(WithArg<0>([&](const void* actualKeyData) { - EXPECT_EQ(memcmp(actualKeyData, expectedKey.data(), expectedKey.size()), 0); + // Expect a call to FindKey with the expected key. + EXPECT_CALL(mMockCache, FindKey(_)) + .WillOnce(WithArg<0>([&](std::span<const std::byte> actualKey) { + EXPECT_EQ(actualKey.size(), expectedKey.size()); + EXPECT_EQ(memcmp(actualKey.data(), expectedKey.data(), expectedKey.size()), 0); return 0; })); @@ -162,7 +163,7 @@ req2.d = UnsafeUnserializedValue(&v2); req2.e = UnsafeUnserializedValue(Foo{24}); - EXPECT_CALL(mMockCache, LoadData(_, _, nullptr, 0)).WillOnce(Return(0)).WillOnce(Return(0)); + EXPECT_CALL(mMockCache, FindKey(_)).WillOnce(Return(0)).WillOnce(Return(0)); static StrictMock<MockFunction<int(CacheRequestForTesting)>> cacheMissFn; @@ -211,7 +212,7 @@ static StrictMock<MockFunction<int(CacheRequestForTesting)>> cacheMissFn; // Mock a cache miss. - EXPECT_CALL(mMockCache, LoadData(_, _, nullptr, 0)).WillOnce(Return(0)); + EXPECT_CALL(mMockCache, FindKey(_)).WillOnce(Return(0)); // Expect the cache miss, and return some value. int rv = 42; @@ -255,10 +256,10 @@ std::as_bytes(std::span(kCachedData))); // Mock a cache hit, and load the cached data. - EXPECT_CALL(mMockCache, LoadData(_, _, nullptr, 0)).WillOnce(Return(actualStoredData.Size())); - EXPECT_CALL(mMockCache, LoadData(_, _, _, actualStoredData.Size())) - .WillOnce(WithArg<2>([&actualStoredData](void* dataOut) { - memcpy(dataOut, actualStoredData.DataPtr(), actualStoredData.Size()); + EXPECT_CALL(mMockCache, FindKey(_)).WillOnce(Return(actualStoredData.Size())); + EXPECT_CALL(mMockCache, LoadData(_, _)) + .WillOnce(WithArg<1>([&actualStoredData](std::span<std::byte> dest) { + memcpy(dest.data(), actualStoredData.DataPtr(), actualStoredData.Size()); return actualStoredData.Size(); })); @@ -305,10 +306,10 @@ std::as_bytes(std::span(kCachedData))); // Mock a cache hit, and load the cached data. - EXPECT_CALL(mMockCache, LoadData(_, _, nullptr, 0)).WillOnce(Return(actualStoredData.Size())); - EXPECT_CALL(mMockCache, LoadData(_, _, _, actualStoredData.Size())) - .WillOnce(WithArg<2>([&actualStoredData](void* dataOut) { - memcpy(dataOut, actualStoredData.DataPtr(), actualStoredData.Size()); + EXPECT_CALL(mMockCache, FindKey(_)).WillOnce(Return(actualStoredData.Size())); + EXPECT_CALL(mMockCache, LoadData(_, _)) + .WillOnce(WithArg<1>([&actualStoredData](std::span<std::byte> dest) { + memcpy(dest.data(), actualStoredData.DataPtr(), actualStoredData.Size()); return actualStoredData.Size(); })); @@ -361,11 +362,11 @@ static StrictMock<MockFunction<int(Blob)>> cacheHitFn; static StrictMock<MockFunction<int(CacheRequestForTesting)>> cacheMissFn; - // Mock a cache hit, but with different sizes returned from LoadData. + // Mock a cache hit, but with different sizes returned from FindKey and LoadData. const size_t kExpectedSize = 10; const size_t kActualSize = 5; - EXPECT_CALL(mMockCache, LoadData(_, _, nullptr, 0)).WillOnce(Return(kExpectedSize)); - EXPECT_CALL(mMockCache, LoadData(_, _, _, kExpectedSize)).WillOnce(Return(kActualSize)); + EXPECT_CALL(mMockCache, FindKey(_)).WillOnce(Return(kExpectedSize)); + EXPECT_CALL(mMockCache, LoadData(_, _)).WillOnce(Return(kActualSize)); // Expect the cache miss handler since the load sizes were different. int rv = 79; @@ -431,12 +432,11 @@ unsigned int* cPtr = req.c.data(); // Mock a cache hit with given data buffer. - EXPECT_CALL(mMockCache, LoadData(_, _, nullptr, 0)).WillOnce(Return(loadSize)); - EXPECT_CALL(mMockCache, LoadData(_, _, _, loadSize)) - .WillOnce(WithArg<2>([&](void* dataOut) { - memcpy(dataOut, loadBuffer, loadSize); - return loadSize; - })); + EXPECT_CALL(mMockCache, FindKey(_)).WillOnce(Return(loadSize)); + EXPECT_CALL(mMockCache, LoadData(_, _)).WillOnce(WithArg<1>([&](std::span<std::byte> dest) { + memcpy(dest.data(), loadBuffer, loadSize); + return loadSize; + })); // Construct mock functions for current test. ASSERT_FALSE(cacheHitFn);