Ensure that Skia persistent cache filenames do not exceed the maximum allowed length #28315
Conversation
|
|
||
| std::string_view view(reinterpret_cast<const char*>(sha_digest), |
There was a problem hiding this comment.
It looks like asan is unhappy with this. Maybe there's an obo in the buffer size?
There was a problem hiding this comment.
The ASAN issue is due to a bug in the Base32 encoder - see #28328
|
|
||
| uint32_t signature = kSignature; | ||
| uint32_t version = kVersion1; | ||
| uint32_t key_size; |
There was a problem hiding this comment.
How long are keys? Could version and size be uint16_t?
There was a problem hiding this comment.
Keys are arbitrarily sized SkData instances provided by Skia, and SkData::size() returns a size_t. In practice the keys are likely to be at most a few hundred bytes long, but I used a uint32_t for safety.
| if (mapping->GetSize() == 0) { | ||
| return nullptr; | ||
| if (mapping->GetSize() < sizeof(CacheObjectHeader)) { | ||
| return result; |
There was a problem hiding this comment.
Should we consider some log messages here and below to flag when the file is in a bad state?
There was a problem hiding this comment.
Added some logs. Corruption is unlikely because these files are written with fml::WriteAtomically, which completes the write and then renames the file if successful.
jason-simmons
force-pushed
the
bug_88726
branch
from
4ccdf5a to
48043ba
Compare
common/graphics/persistent_cache.cc
Outdated
|
|
||
| std::string_view view(reinterpret_cast<const char*>(sha_digest), | ||
| SHA_DIGEST_LENGTH); | ||
| auto encode_result = fml::Base32Encode(view); |
There was a problem hiding this comment.
Base32 encoding the key seems unnecessary now after the digest has been calculated. Can we just convert it to a hex string using standard utilities and get rid of fml::Base32Encode and Decode altogether? Will just simplify things.
There was a problem hiding this comment.
Added a hex encode function to FML, which should be slightly faster than Base32. Base32 is still needed for reading and writing the precompiled shader JSON format.
There was a problem hiding this comment.
Oh, right. I forgot about the JSON for training. Sounds good.
| @@ -291,17 +293,38 @@ bool PersistentCache::IsValid() const { | |||
| return cache_directory_ && cache_directory_->is_valid(); | |||
| } | |||
|
|
|||
| sk_sp<SkData> PersistentCache::LoadFile(const fml::UniqueFD& dir, | |||
| const std::string& file_name) { | |||
| PersistentCache::SkSLCache PersistentCache::LoadFile( | |||
There was a problem hiding this comment.
Minor nit: Can we make PersistantCache::SkSLCache a struct with named members instead of a pair? It's hard to read now because both items are SkData and its not clear if the key is first or the value.
common/graphics/persistent_cache.h
Outdated
| static const uint32_t kSignature = 0xA869593F; | ||
| static const uint32_t kVersion1 = 1; | ||
|
|
||
| CacheObjectHeader(uint32_t _key_size) : key_size(_key_size) {} |
There was a problem hiding this comment.
Minor nit: The underscore position is not conventional. Perhaps p_key_size?
shell/common/shell.cc
Outdated
| @@ -1685,8 +1686,13 @@ bool Shell::OnServiceProtocolGetSkSLs( | |||
| SkBase64::Encode(sksl.second->data(), sksl.second->size(), b64_char); | |||
| b64_char[b64_size] = 0; // make it null terminated for printing | |||
| rapidjson::Value shader_value(b64_char, response->GetAllocator()); | |||
| rapidjson::Value shader_key(PersistentCache::SkKeyToFilePath(*sksl.first), | |||
| response->GetAllocator()); | |||
| std::string_view key_view(reinterpret_cast<const char*>(sksl.first->data()), | |||
There was a problem hiding this comment.
Per the comment I made above about making SKSLCache a struct. I can't tell what sksl.first means here without some context. Just a minor nit.
jason-simmons
force-pushed
the
bug_88726
branch
2 times, most recently
from
4d8582b to
f9eb85b
Compare
… allowed length The cache will now use a hash of the entry's key as the filename. The full key will be stored inside the cache file. Fixes flutter/flutter#88726
jason-simmons
force-pushed
the
bug_88726
branch
from
f9eb85b to
79207ca
Compare
jason-simmons
added
the
waiting for tree to go green
… maximum allowed length (flutter/engine#28315)
|
@jason-simmons SkiaPerf is showing that this change gave a very nice improvement in worst case frame rasterization time on our iOS sksl warmup benchmark. Thanks, Jason! |
The cache will now use a hash of the entry's key as the filename.
The full key will be stored inside the cache file.
Fixes flutter/flutter#88726