Distributed size for concurrent ordered containers#1803
Conversation
| key_compare my_compare; | ||
| random_level_generator_type my_rng; | ||
| atomic_node_ptr my_head_ptr; | ||
| std::atomic<size_type> my_size; |
There was a problem hiding this comment.
There is also an option to keep my_size, reserve a flag bit in it and use it for optimizing consecutive calls to size() by saving the result of combine into my_size and marking it as "changed" in the insertion.
I did not investigate potential performance effects from CAS operations on my_size, just wanted to save the idea.
kboyarinov
requested review from
aleksei-fedotov,
dnmokhov,
isaevil and
vossmjp
|
|
||
| void set_size(size_type size) { my_local_size.store(size, std::memory_order_relaxed); } | ||
| void increment_size() { | ||
| my_local_size.store(local_size() + 1, std::memory_order_relaxed); |
There was a problem hiding this comment.
Why not use atomic std::atomic<>::fetch_add instead of load and store pair?
There was a problem hiding this comment.
My understanding is that is done to avoid locking the cache line, since only one thread writes to this atomic anyway. I think that it would be good to leave a small comment that explains the motivation.
There was a problem hiding this comment.
Definitely, the comment will be useful.
There was a problem hiding this comment.
Added the comment
Signed-off-by: Isaev, Ilya <ilya.isaev@intel.com>
…ributed_size_combined
include/oneapi/tbb/concurrent_set.h
Outdated
| Copyright (c) 2019-2025 Intel Corporation | ||
| Copyright (c) 2025 UXL Foundation Contributors |
There was a problem hiding this comment.
I believe the copyright is outdated hier and in other files.
| atomic_node_ptr my_head_ptr; | ||
| std::atomic<size_type> my_size; | ||
| std::atomic<size_type> my_max_height; | ||
| mutable ets_type my_ets; |
There was a problem hiding this comment.
Is mutable needed here because of ets::combine method being non-const?
There was a problem hiding this comment.
Yes, added the comment
|
|
||
| using random_level_generator_type = typename container_traits::random_level_generator_type; | ||
| using thread_data_type = skip_list_thread_data<random_level_generator_type, size_type>; | ||
| using ets_type = tbb::enumerable_thread_specific<thread_data_type>; |
There was a problem hiding this comment.
Why should not this be the ETS with ets_suspend_aware?
There was a problem hiding this comment.
Thanks, I agree that it will be more flexible from the user standpoint.
Applied to check performance effects.
aleksei-fedotov
left a comment
aleksei-fedotov
left a comment
There was a problem hiding this comment.
I think it is good enough in general.
Found just a small oversight in the comment.
Co-authored-by: Aleksei Fedotov <aleksei.fedotov@intel.com>
|
Latest performance measurements (LibCuckoo universal benchmark) confirmed the performance improvement on the latest version of the patch. So, I guess it can be merged. |
4 participants
Description
Add a comprehensive description of proposed changes
Fixes # - issue number(s) if exists
Type of change
Choose one or multiple, leave empty if none of the other choices apply
Add a respective label(s) to PR if you have permissions
Tests
Documentation
Breaks backward compatibility
Notify the following users
List users with
@to send notificationsOther information