Cache fetching rotated key names #498
Conversation
…lls to storage backend some optimizations related to compiling filenames cache poison record keys and optimize fetching with removed extra storage checks
Co-authored-by: Anatolii Lishchynskyi <anatolii.lishchynskyi@cossacklabs.com>
| @@ -387,12 +390,55 @@ func (store *KeyStore) GetPublicKeyFilePath(filename string) string { | |||
| return fmt.Sprintf("%s%s%s", store.publicKeyDirectory, string(os.PathSeparator), filename) | |||
| } | |||
|
|
|||
| // use key started with "." (dot) because it's invalid character for clientID that generally stored in cache and | |||
| // it will not intersect with other keys | |||
| const cacheKeySuffix = ".historical." | |||
There was a problem hiding this comment.
Based on how you use it, it's more like a cacheKeyPrefix 🤔
|
We probably could make keystore cache interface to be |
| // goos: linux | ||
| // goarch: amd64 | ||
| // pkg: github.com/cossacklabs/acra/keystore/filesystem | ||
| // cpu: Intel(R) Core(TM) i7-10750H CPU @ 2.60GHz |
There was a problem hiding this comment.
Did you lock CPU frequency at this specific (or some other) value when benchmarking?
BTW, to get more precise results, instead of closing other apps, sending them SIGSTOP will also do the trick 👍
There was a problem hiding this comment.
for what we need here such precise benchmarks? there are 10^10 difference between approaches. you think such improvements may change general picture of that and we should spent time for that?)
There was a problem hiding this comment.
it's just simple benchmark to show other developers who will read code and think: why you used this serialization algo instead of some more simple from std. and to allow everyone reproduce by one command on their machine to be sure
There was a problem hiding this comment.
Just saying what comes in mind. Here we clearly see that msgpack is much better, that's enough.
BTW, if you wanted to try something like this, I got a shell function to set min/max frequencies, at least it worked for some Intel laptop CPU
set-cpu-freq-min-max() {
[[ $1 > 0 && $2 > 0 ]] || { echo "$0 <min_freq_KHz> <max_freq_KHz>"; return }
echo $1 | sudo tee /sys/devices/system/cpu/cpu*/cpufreq/scaling_min_freq
echo $2 | sudo tee /sys/devices/system/cpu/cpu*/cpufreq/scaling_max_freq
}
I though about it... but I don't want to use reflection here because it more type checks, more code like: for now, it is enough to convert everything to bytes, imho. you can add more arguments why we should change this approach. |
add unit-tests that catch this issues
store.getCachedHistoricalPrivateKeyFilenamescall that reads folder every time when decryption keys are requested.[]bytetype used in our in-memory cache with interfaceGet(id[]byte)[]byte/Set([]byte, []byte), to cache results used msgpack as more efficient way to serialize/deserialize thangoborsplit/joinstrings.internalpackage because msgpack can serialize only public structs. And this struct shouldn't be public and exported fromkeystore/filesystempackage. So to hide and encapsulate this struct, it placed tointernalthat accessible fromkeystore/filesystem, but not accessed from top-level packages.getSymmetricKeysdue to small start allocationkeys := make([][]byte, 0, 1)when usually may be at least 2 keys (current and 1 rotated). So increased to 4. It is not increase memory usage because it's allocates +3 structs with 3intfields (SliceHeader with ptr + length + capacity), so it's just 3 * 24 bytes (72) instead of 24.Checklist
with new changes