Libcds module #4909
Libcds module #4909
Conversation
libs/libcds/tests/performance/libcds_hazard_pointer_overhead.cpp
Outdated
Show resolved
Hide resolved
libs/libcds/tests/performance/libcds_hazard_pointer_overhead.cpp
Outdated
Show resolved
Hide resolved
libs/libcds/tests/unit/libcds_feldman_map_hazard_pointer_stdthread.cpp
Outdated
Show resolved
Hide resolved
libs/libcds/tests/unit/libcds_feldman_map_hazard_pointer_stdthread.cpp
Outdated
Show resolved
Hide resolved
libs/libcds/tests/unit/libcds_flat_combining_priority_queue_stdthread.cpp
Outdated
Show resolved
Hide resolved
weilewei
force-pushed
the
libcds-module
branch
from
387adc6
to
0d180d7
Compare
No problem. I also want an RAII wrapper for the main init. I'm hoping @weilewei can take care of the issues here during this week and have everything ready before the end of GSoC |
|
Yes, I will try my best to take care of issues this week, and please expect my fixes coming on Thursday or Friday. Thanks. @biddisco |
|
I suggest we leave this out of 1.5.0 by the way, simply because it's very late in the release process and it's not intended to for public use yet. Let's discuss at the meeting if someone would still prefer to have this in 1.5.0. |
There was a problem hiding this comment.
I believe I fixed most of the comments, here two important wrappers are added
- is
libcds_wrapperthat libks tocds::Initialize()andcds::Terminate() - is templated
hazard_pointer_wrapperthat links to calling hazard pointer constructorcds::gc::hp::custom_smr<TLS>::construct(..)
also, we already have hpxthread_manager_wrapper that deals with ::attach_thread() and ::detach_thread(), which keeps track of number of threads attached to hazard pointer object.
I would suggest the 2nd wrapper should not bind to 1st wrapper, as they are quite independent. Having hazard pointer initialized is a subset of libcds initialization, as libcds can have other types, such as dynamic hazard pointer, rcu which we don't have support yet.
Please let me know any other suggestions. Thanks.
libs/libcds/tests/unit/libcds_feldman_map_hazard_pointer_stdthread.cpp
Outdated
Show resolved
Hide resolved
|
I moved hazard pointer init into libcds init now using enum switch, please review it again. Thanks. |
|
Now all map tests are in one single file, now only showing hpxthread and stdthread tests for map. I plan to add hpx::init around Wednesday as well as update doc. |
|
Please review it again, thanks. I have added command line argument option to update max attached concurrent thread for hp, for example: --hpx:ini=hpx.cds.num_concurrent_hazard_pointer_threads=256. Let me know what do you think. |
cmake/HPX_SetupLibCDS.cmake
Outdated
| set_target_properties(cds-s PROPERTIES FOLDER "Core") | ||
| # create an imported target that links to the real libcds so that when we | ||
| # link to the imported target, we don't get export X depends on cds that is | ||
| # not in the export set |
There was a problem hiding this comment.
Is this warning just about export sets? Does libcds get installed alongside HPX?
Having libcds as a "normal" purely external dependency would remove the burden from us to do it correctly (we might need a FindLibcds.cmake module though).
There was a problem hiding this comment.
This question goes to @biddisco
There was a problem hiding this comment.
I was hoping nobody would ask about the install rules. libcds is not in the export set of hpx, so it won't get installed alongside it. If we are able to use libcds master, then I'm happy to simply have an external libcds that the user must point the build at using cmake -DLIBCDS_ROOT=... which removes the need to install ourselves - but if we require a dedicated branch (currently, we need Alex's hp_generic branch, then the in-source build is preferred. For now I'm happy to leave things as they are and ignore the install problem since only us developers are using it. If I can't solve it - then I'll switch to an external libcds install when latest stuff is merged to libcds master.
There was a problem hiding this comment.
I'm glad I asked then! Yeah, I'm also ok with keeping this as they are at the moment, but I think long term having it as a normal external dependency is the only right thing to do.
libs/libcds/docs/index.rst
Outdated
| int main(int argc, char* argv[]) | ||
| { | ||
| return hpx::init(argc, argv); | ||
| } |
There was a problem hiding this comment.
Please move this to the examples directory, and add the file with literalinclude instead. Getting the path correct in the literalinclude is a bit tricky, so I can help with that part.
There was a problem hiding this comment.
Can you help with this? I am not sure what literalinclude is. Thanks.
There was a problem hiding this comment.
Yeah, sure. I can push a change for this.
There was a problem hiding this comment.
I have a commit for this, but it's on another laptop. I'll push it next week.
|
@weilewei could you mark the comments that you've already taken care of as resolved, please? |
| @@ -342,7 +300,7 @@ int main(int argc, char* argv[]) | |||
|
|
|||
| // clang-format off | |||
| cmdline.add_options() | |||
| ("futures", value<std::uint64_t>()->default_value(500000), | |||
| ("futures", value<std::uint64_t>()->default_value(50000), | |||
There was a problem hiding this comment.
Was there a particular reason for changing the default here? Should the default be changed in the normal future overhead test as well? Or does this one run significantly slower than the normal one that it makes sense to reduce this?
There was a problem hiding this comment.
@biddisco reduced the number in order to reduce testing time, maybe he can answer it.
But to my understanding, launching more futures (i.e. 50k or 500k) than concurrently allowed attached threads (128 by default) is not a helpful testing, because the rest of threads (if there is already 128 threads running inside Hazard Pointer containers at one moment, and then the rest is 50k - 128) are not allowed to go into hazard pointer. So testing 128 futures might be good enough.
There was a problem hiding this comment.
It will be great for you to add a callback where appropriate, as I am not sure what does the callback looks like and how to add it.
There was a problem hiding this comment.
In other tests we use different number for Debug and Release configurations. One is for performance measurements, the other one for making sure things actually work without having to run for a long time.
| class HPXDataHolder | ||
| { | ||
| public: | ||
| CDS_EXPORT_API static CDS_EXPORT_API thread_data* getTLS(); |
There was a problem hiding this comment.
Two questions:
- Should this actually be using
HPX_EXPORTinstead? - Can the code be in
hpx::cdsinstead ofcds::...? This is in the end our code, not libcds code.
There was a problem hiding this comment.
For 1, in LibCDS code, it requires CDS_EXPORT_API so I was following their style.
For 2, @biddisco maybe have more ideas about it.
There was a problem hiding this comment.
Do we need the macro twice? Also, whether we should be using HPX_EXPORT or CDS_EXPORT_API depends on what shared library the implementation of this function ends up being linked to.
Sure! Just marked |
| std::size_t hazard_pointer_count = 1, | ||
| std::size_t max_thread_count = | ||
| detail::get_num_concurrent_hazard_pointer_threads(), | ||
| std::size_t max_retired_pointer_count = 16) |
There was a problem hiding this comment.
I've added the callback here. My question now is if those other parameters should have configuration options as well or not? Is this the appropriate order for the default parameters (i.e. which ones are the user most likely to change)?
There was a problem hiding this comment.
FWIW, having three size_t arguments is highly confusing. Nobody will be able to remember what sequence those have to be supplied in.
There was a problem hiding this comment.
@weilewei this is a tough question. size_t is the right type to use. The problem is that it's difficult to pass the values in a correct sequence. One possible way to enforce the user has to use names would be to define three structs, like:
struct hazard_pointer_count
{
explicit hazard_pointer_count(std::size_t value) : value_(value) {}
private:
std::size_t value_;
};
and use those as:
struct libcds_wrapper
{
libcds_wrapper(smr_t smr_type = smr_t::hazard_pointer_hpxthread,
hazard_pointer_count count = hazard_pointer_count(1),
max_thread_count max_threads = max_thread_count(
detail::get_num_concurrent_hazard_pointer_threads()),
max_retired_pointer_count retired_count = max_retired_pointer_count(16))
{
// ...
}
};
|
|
||
| }}}} // namespace cds::gc::hp::details | ||
|
|
||
| namespace hpx { namespace cds { |
There was a problem hiding this comment.
| namespace hpx { namespace cds { | |
| namespace hpx { namespace cds { namespace experimental { |
?
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
- Fix initialization of RCU threads in RAII wrapper - Add callback for cds configuration option - Fix failing test by setting concurrent thread limit - remove unnecessary include guard - move export macro to declarations - fix Mickael's suggestion - make cmake format happy - update doc and fix typo - now get_config_entry for libcds is fixed and populated to tests - WIP remove max current thread variable - WIP adding config option for num threads hp - adapt libcds changes, from tls to dataholder, and hpx-1.5 tag in libcds - move all map tests into one single file - fix typo in rcu example - make inspect happy - fix rcu init and add error protection to hp attach/detach threads - use enum in libcds_wrapper to support hp, dhp, rcu, remove hazard_pointer_wrapper - WIP modify one test to meet John's request - make inspect happy - update doc with latest changes - create Hazard Pointer RAII wrapper and apply to all test cases - adding libcds init RAII wrapper - use thread_manager_wrapper everywhere - replacing assert to HPX_ASSERT - mostly minor fixes to libcds wording - minor fix by Alex's suggestion - minor fix on libcds doc - try to fix codeblock rendering in Github - add headings to libcds doc - adding libcds documentation - Removing non-needed libCDS support functions (for now) - Fix TEST_EQ check in flat_combining_priority_queue test - Reduce default number of threads in HP overhead test to minimize test time - Upgrade flat-combining test and add std::thread RAII wrapper for libCDS - The flat combining priority queue test now generates unique numbers in random order for N threads and inserts them into a queue, then checks that they are taken off in valid order. - Add stdthread wrapper for libcds thread init and rename other to hpxthread_Wrapper to distinuguish between them - Fix thread counter checks (counter is unsigned, so val < 0 is not useful) - Remove SOURCE_PATH_ABSOLUTE that is not needed for libCDS build any more - modify thread counter increment - Rename libcds tests from Mickael to Michael - add default tls manager tests, related to map container - add max concurrent thread init variable to libcds - Move libcds RAII thread manager wrapper into main module header - Update tests to use wrapper object - Remove files and cmake settings not needed for templated libcds - Link to cds and fix CMake EXPORT problem - First version of module build of libcds took the source files from libcds and compiled them into the module - but this required a tweak to the libcds CMakeLists. - Instead, we build libcds and then create an import interface target that we link to which solves the problem of EXPORT not being in the target set if we link directly to cds - Fix naming of hpx_tls_manager header and thread_data namespace issue - WIP try fetch libcds master branch and use hpxtls manager in libcds module - split_list_map_hazard_pointer - add feldman_map_hazard_pointer example - add random sleep for hpx threads, and format codes - adding mickael_map_hazard_pointer example - adapt hp overhead test to templated tls manager style - fix libcds fc-pq example and remove hpx threading in it - remove hard-coded libcds repo fetch of hpx-module - Compile libCDS into an HPX module - adapt hpx header changes - minior modification to fc pq example - adding libcds deque example - adjust #ifdef to #if define(X) style - remove unecessary static data, headers, move preprocessor directives, rename test file - add error handling if libcds is not fetched successfully - use value semantics and descriptive function names - separate std::array into individual elements same as threading functions - update CXX standard for libcds due to its master changes - typedef std::array for libcds - fix PR comments related to cmake and headers order - clang-format cmake-format - add Hazard pointer overhead test - use std::array to maintain libcds data instead of vector - add vector of size_t for libcds - adding libcds thread data functions - clean up libcds cmake - move libcds setup back - adding initial support for libcds option
|
FWIW, the next step is to go through the review comments and apply necessary changes. I have not looked at the review comments and whether those have been addressed yet. |
hkaiser
force-pushed
the
libcds-module
branch
2 times, most recently
from
980e2a4
to
983b5a2
Compare
|
Closing as this is inactive. |
GSoC project - merge
hpxbased hazard pointer andstd::threadbased libCDS work