-
Notifications
You must be signed in to change notification settings - Fork 346
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
make wt_int and wm_int thread safe #264
base: master
Are you sure you want to change the base?
Conversation
This is accomplished by moving m_path_off and m_path_rank_off to the local context of select (see #263). Now, grep mutable include/sdsl/*hpp yields only wt_pc.hpp, bp_support_sada, and csa_sada, all of which have shared caches that will not be thread safe without changes to the API.
Can one of the admins verify this patch? |
I think it might make sense to revert my last push. Trying to understand what I'm seeing here once I try to integrate the changes into another application: https://travis-ci.org/ekg/xg/builds/78046876#L5603-L5645. This wasn't an issue with Here's the text of the error in case it vanishes into travis history:
|
why are you passing an argument to the constructor of std::array? The 65 as 2015-08-31 16:22 GMT+02:00 Erik Garrison [email protected]:
|
I'd like to pick this back up as it's continuing to cause issues for me. Relative to the patch that I proposed, what changes would you require to make this acceptable? I'm also interested in doing something similar for csa_sada. |
just making the buffer in csa_sada local would probably cause lots of allocations which will degrade performance? Have not benchmarked this though. |
It seems it might hurt performance. That said, mutex locking isn't a great solution. What might be sensible is to pass a thread-local buffer into the select. |
Yes, a third argument which is a pointer to a thread-local buffer seems to be the good solution, since it is backward-compatible. If a nullptr is passed we could then just use the old mutable array to still get the same performance for single-threaded program ;) |
the buffer is also used in rank_bwt which is called from the suffix_array_helper classes which are called from count() and extract() etc. so it will be quite difficult to adjust all of these and remain backward-compatible. |
Yes, it is used in those places, but since it is only called with the two arguments it will still work as expected in the single-threaded case. You are certainly right that more adjustments have to be made to make some methods of the CSA (like rank_bwt,count, and extract) work in a multithreaded environment, but I think this is not the point of Erik's request. |
well csa_sada::select_bwt() doesn't use the buffer at all so not sure what the problem is here? |
also, there is a assert(cc != 255); in there which probably has to removed as we now deal with integer alphabets. |
How much does the caching help? Could I simply add an option to turn it off to all of the classes that use this kind of non-threadsafe feature? |
This is accomplished by moving m_path_off and m_path_rank_off to the local
context of select (see #263).
Now, grep mutable include/sdsl/*hpp yields only wt_pc.hpp, bp_support_sada, and
csa_sada, all of which have shared caches that will not be thread safe without
changes to the API.