ValueMap - separate HashMap for sorted attribs #2288
Conversation
fraillt
force-pushed
the
value-map-separate-map-for-sorted-attribs
branch
from
b4ee62f
to
c9621ae
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2288 +/- ##
=======================================
- Coverage 79.4% 79.4% -0.1%
=======================================
Files 122 122
Lines 20783 20786 +3
=======================================
+ Hits 16506 16507 +1
- Misses 4277 4279 +2 ☔ View full report in Codecov by Sentry. |
|
Benchmark results: cargo bench --bench metricIt looks like there's relation between number number of attributes, E.g. for 1 attribute, performance is increased, for 10 is decreased, but I cannot explain why this happens. cargo run --release --package stress --bin metricsSame theme as before, counter performance increase, histograms performance decrease. |
| let trackers = match self.sorted_attribs.lock() { | ||
| Ok(mut trackers) => { | ||
| let new = HashMap::with_capacity(trackers.len()); | ||
| replace(trackers.deref_mut(), new) |
There was a problem hiding this comment.
We have briefly touched this situation in my previous PR. about unnecessary allocation, but we didn't discussed downsides of it.
Here's the table of possible solutions.
| memory policy | approach | benefits | drawbacks |
|---|---|---|---|
| concervative | take(trackers) | smallest memory footprint never grows more than needed | lots of allocation every time new collection phase starts |
| balanced | replace(trackers, HashMap with_capacity (previous_len)) | also small memory footprint | on stable loads might save a lot of unnecessary allocations, on spiky loads might either waist memory or do more allocations |
| liberal | replace(trackers, previous_tracker with same_memory) | no allocations at all | memory only grow, which might be a problem if you have huge spike, allocated memory will stay there forever (though it's only for contents of "empty" buckets, maybe not that bad?). Also it's a bit more complexity in code, as you need to have extra variable (and lock) just for that, and finally this might be unexpected for end user, as end user might expect that delta temporality doesn't do any sort of caching |
At the moment I chose "balanced approach", I found it most natural :) but happy to hear your opinion on this too.
| prepare_data(dest, self.count.load(Ordering::SeqCst)); | ||
| if self.has_no_attribute_value.swap(false, Ordering::AcqRel) { | ||
| // reset sorted trackers so new attributes set will be written into new hashmap | ||
| let trackers = match self.sorted_attribs.lock() { |
There was a problem hiding this comment.
I believe this is making the contention between collect and update worse. Earlier, the only point of contention during collect was the write lock under which we replace the existing RwLock<Hashmap> with a default one. With the changes in this PR, we now have two points of contention during collect:
- Locking the
Mutexwithself.sorted_attribs.lock()which would compete with updates trying to add newer attribute sets - Acquiring the write lock with
self.all_attribs.write()which would compete with all the updates
There was a problem hiding this comment.
Yes, I know, but I don't see other way if we want to implement sharding, so my goal with this PR was mostly try to preserve existing performance (as much as I could) while at the same time, make it easy to add sharding in the future.
However, I expect this extra lock to be very cheap in practice, for two reasons:
- Mutex is generally fast
- at the end of collection cycle, there should be quite low contention on inserting new attribute sets.
There was a problem hiding this comment.
but I don't see other way if we want to implement sharding,
Why is that? Implementing sharding for the Hashmap should be only about replacing the existing HashMap type with the type that implements sharding (something like Dashmap). How is the lack of having a sorted and unsorted set of attributes trackers preventing us from trying a sharded Hashmap?
There was a problem hiding this comment.
Everything could work with one hashmap, we could probably tolerate slower cumulative collection phase, but the real problem is with delta temporality collection.
Here's the problem:
We have hard requirement that attribute-set keys are order agnostic.
In practice this means, that we can have multiple attribute-sets in hashmap, which points to same tracker (hence, we have Arc<A>.).
With sharding this means, that same tracker can be in multiple shards, so the question is how do you reset hashmap?
- we cannot simply "replace" hashmap with fresh instance.
- we cannot iterate through each shard, deduplicate (using hashset to identify already processed attribute-sets), and reset/clear each shard while iterating. This doesn't work because, even if you remove attribute-set in one shard, on next tracker update same tracker instance will be put back, as long as sorted attribute-set instance lives in another shard.
The only option to actually clear sharded hashmap is to lock ALL shards, then iterate/clone and unlock.
So my approach is to have two hashmaps instead, because all things considered it has more benefits than drawbacks.
- update existing tracker - no effect, because attribute set is found on first try in both cases (in case user never provides sorted attribute-sets, it might be even better, as first hashmap will be half as small, so less chance for collision).
- inserting new tracker - probably not expected, but it is performance increase actually (with cargo bench --bench metrics_counter), probably due to less checks and fast lock with Mutex.
- collection phase (both delta and cumulative) - huge benefits, short lock time, no need to have hashset in order to dedup, and solves Fix metrics dedup/sort bug #2093 (which would further slow down collection phase).
So basically this approach has bunch of benefits and 0 drawbacks, so why not try it :)
There was a problem hiding this comment.
I understand that sharding has proven to be improving throughout significantly, but those tests were done at the time we took a Mutex lock in hotpath. Now we have replaced that with Read lock on hotpath, so there is no contention anymore. i.e if 10 threads wants to update(), they can all do so without being blocked by each other. With this existing design, how does sharding help improve performance?
There was a problem hiding this comment.
I think it has to do with CPU cache contention.
There's still single lock instance (on happy path) that all cores/threads will fight for. It doesn't matter if it's Mutex/RwLock or any other primitive, the fact remains that when we "lock it" some state is updated under the hood.
Once one CPU updates a memory (there's no difference if it's atomic or not), it marks this L1 cache line as "invalid" on all other CPU cores, so when next core comes in, it needs to fetch it (might be from main memory or previous CPU cache).
So sharding solves exactly this problem:
- it will have multiple lock instances (per shard), so that multiple threads don't fight that much anymore, and don't need to invalidate other CPU cache line every single time.
As I mentioned, I have done some tests locally, using naive implementation (I didn't used any padding between shards, to avoid false-sharing (when invalidating one shard, might also invalidate another if they happen to live on same L1 cache line)), and results are already ~3x improvement (it varies greatly between different CPUs, I saw 2x on one, and 5x on another in some cases). Proper implementation could be even more performant :)
|
Some more benchmark results: cargo bench --bench metrics_counter |
Changes
Internal structure for ValueMap has changed, in order to achieve several things:
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial, user-facing changes