Storage lock increase wait time and add artificial slow writes #2497
Conversation
…st_slow_writes_tests
…st_slow_writes_tests
Storage lock no longer does an additional check if object exists but instead only calls GetObject
…st_slow_writes_tests
ognyanstoimenov
added
patch
cpp/arcticdb/util/configs_map.hpp
Outdated
| map_of_##LABEL[boost::to_upper_copy<std::string>(label)] = val; \ | ||
| } \ | ||
| \ | ||
| TYPE get_##LABEL(const std::string& label, TYPE default_val) const { \ | ||
| std::lock_guard<std::mutex> lock(mutex_); \ |
There was a problem hiding this comment.
Why do we need this guard? We call these get_ methods all the time, often in fairly performance sensitive blocks (which perhaps we shouldn't), so I'm a bit concerned about the performance impact
There was a problem hiding this comment.
I agree these locks shouldn't be needed. It could only be useful for tests which are setting and getting values from multiple threads. Such tests need better design.
Some general notes, not suggesting we do any of this:
The mutex lock without contention is quite cheap. but in this use case there could be contention from multiple readers whichmight explain the failing ASV benchmark. There exist single-writer multiple-reader primitives which can resolve this.
Still even just hashing the string every time and accessing the hashmap can be expensive but should again be on the order of tens of nanoseconds (especially if compiler is smart enough to hash the string compile time).
I do not think we really use the configsmap in settings where 100nanoseconds matter.
There was a problem hiding this comment.
Perhaps this benchmark regression is real. Will be good to get to the bottom of it.
| Change | Before [91d18651] | After [66842fe3] | Ratio | Benchmark (Parameter) |
|----------|----------------------|---------------------|---------|-----------------------------------------------------------------------------------|
| + | 168±2ms | 194±4ms | 1.16 | basic_functions.BatchBasicFunctions.time_read_batch_with_date_ranges(25000, 500) |
| + | 188±2ms | 218±5ms | 1.16 | basic_functions.BatchBasicFunctions.time_read_batch_with_date_ranges(50000, 500) |
| + | 351±4ms | 404±8ms | 1.15 | basic_functions.BatchBasicFunctions.time_read_batch_with_date_ranges(25000, 1000) |
cpp/arcticdb/util/configs_map.hpp
Outdated
| map_of_##LABEL[boost::to_upper_copy<std::string>(label)] = val; \ | ||
| } \ | ||
| \ | ||
| TYPE get_##LABEL(const std::string& label, TYPE default_val) const { \ | ||
| std::lock_guard<std::mutex> lock(mutex_); \ |
There was a problem hiding this comment.
I agree these locks shouldn't be needed. It could only be useful for tests which are setting and getting values from multiple threads. Such tests need better design.
Some general notes, not suggesting we do any of this:
The mutex lock without contention is quite cheap. but in this use case there could be contention from multiple readers whichmight explain the failing ASV benchmark. There exist single-writer multiple-reader primitives which can resolve this.
Still even just hashing the string every time and accessing the hashmap can be expensive but should again be on the order of tens of nanoseconds (especially if compiler is smart enough to hash the string compile time).
I do not think we really use the configsmap in settings where 100nanoseconds matter.
| (0.3, 10, 50), | ||
| (0.3, 100, 300), | ||
| (0.3, 300, 500), | ||
| (0.3, 700, 1200) |
There was a problem hiding this comment.
I think with the current setup the last fixture parameter will bring enough slowness to introduce a genine test failure.
I do think this test is useful for when we eventually do something about the symbol list, so we shouldn't remove it but probably xfail it.
Also could you (after xfail-ing) run the real storage test to see this test indeed fails and will prove itself useful for future symbol list development?
|
The lock was indeed the problem for the asv degradation: with lock vs without lock My main concern is this relies on us being disciplined not to write anything to the configmap in multithreaded context and only read from it, while writing is done upfront |
Reference Issues/PRs
What does this implement or fix?
In continuation of #2359 which was reverted. This is the same but without the check for slow writes, as that was causing the lock to never be taken on inherently slow storages.
This is the diff with the reverted PR
symbol_list_slow_writes_tests...symbol_list_slow_writes_tests_after_revert
Changelog is similar to #2359:
Locking gives up on slow writesAny other comments?
Checklist
Checklist for code changes...