Skip to content
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

Made the mem_cache_block lockfree #23

Merged
merged 4 commits into from Jan 10, 2016
Merged

Made the mem_cache_block lockfree #23

merged 4 commits into from Jan 10, 2016

Conversation

@ylow
Copy link
Contributor

@ylow ylow commented Dec 9, 2015

This significantly improves parallel performance of regex.
Currently if I have a large number of threads all using regexes; even if
they are using idependent regex objects, performance is still extremely poor
due to the lock inside of the mem_block_cache.

This *significantly* improves parallel performance of regex.
Currently if I have a large number of threads all using regexes; even if
they are using idependent regex objects, performance is still extremely poor
due to the lock inside of the mem_block_cache.
@jzmaddock
Copy link
Collaborator

@jzmaddock jzmaddock commented Dec 9, 2015

This is a good idea, but the patch isn't completely portable just yet. For preference I would like a solution which:

  • Uses when BOOST_NO_CXX11_HDR_ATOMIC is not set. The code will then need to check ATOMIC_POINTER_LOCK_FREE == 2 before using atomic pointers, otherwise:
  • Uses Boost.Atomic, but checks BOOST_ATOMIC_POINTER_LOCK_FREE==2 before attempting to use atomics. Otherwise:
  • Falls back to the original implementation.
@ylow
Copy link
Contributor Author

@ylow ylow commented Dec 11, 2015

sounds good. I will update the pull request.

@ylow
Copy link
Contributor Author

@ylow ylow commented Dec 15, 2015

I have updated with the suggested checks.

@@ -19,17 +19,72 @@
#define BOOST_REGEX_V4_MEM_BLOCK_CACHE_HPP

#include <new>
#ifdef BOOST_HAS_THREADS

This comment has been minimized.

@jzmaddock

jzmaddock Dec 16, 2015
Collaborator

This include is still needed when neither not <boost/atomic.hpp> provide the functionality we require.

#ifdef BOOST_HAS_THREADS
#include <boost/regex/pending/static_mutex.hpp>
#endif
#include <boost/atomic/atomic.hpp>

This comment has been minimized.

@jzmaddock

jzmaddock Dec 16, 2015
Collaborator

This include should be inside the #ifdef BOOST_NO_CXX11_HDR_ATOMIC block to avoid unnecessary inclusion.

This comment has been minimized.

@ylow

ylow Dec 16, 2015
Author Contributor

Is there a header that should be included for BOOST_NO_CXX11_HDR_ATOMIC ?

This comment has been minimized.

@jzmaddock

jzmaddock Dec 17, 2015
Collaborator

Is there a header that should be included for BOOST_NO_CXX11_HDR_ATOMIC ?

boost/config.hpp - it's already included.

@@ -191,7 +191,9 @@ BOOST_REGEX_DECL void BOOST_REGEX_CALL put_mem_block(void* p)

#else

#ifdef BOOST_HAS_THREADS
#if defined(BOOST_REGEX_MEM_BLOCK_CACHE_LOCK_FREE)
mem_block_cache block_cache;

This comment has been minimized.

@jzmaddock

jzmaddock Dec 16, 2015
Collaborator

We have a problem here: if a regex object is constructed and used at program startup then mem_block_cache may be used uninitialised (since order of initialization is undefined). For std::atomic we can statically initialize, as with the old code, for boost:atomic we can't unfortunately :(

Only workaround I can think of at present is to replace the global variable with a function returning a reference to the cache:

mem_block_cache& get_cache()
{
static mem_block_cache cache;
return cache;
}

Which would also require changing a lot of other code, and making sure the above function is called at program startup to avoid race-conditions when it is eventually called.

There is another issue with boost:atomic, a default initialized atomic as in your code above, actually leaves the value uninitialised, not zero initialized as we require (checked on VC14). Which complicates things even more! I'm not sure what to do about this at present, any thoughts?

This comment has been minimized.

@ylow

ylow Dec 16, 2015
Author Contributor

Hmm.... That's messy. A possibility is this:

Change the constructor of mem_block_cache to a member function, say... initialize()

Add a std::once_flag to the mem_block_cache object.

In the get() and put() function use std::call_once to call initialize()

Assuming std::call_once / boost::call_once has a well implemented fast-path, I think this should work and maintain performance.

This comment has been minimized.

@jzmaddock

jzmaddock Dec 17, 2015
Collaborator

On 16/12/2015 19:37, Yucheng Low wrote:

In src/regex.cpp
#23 (comment):

@@ -191,7 +191,9 @@ BOOST_REGEX_DECL void BOOST_REGEX_CALL put_mem_block(void* p)

#else

-#ifdef BOOST_HAS_THREADS
+#if defined(BOOST_REGEX_MEM_BLOCK_CACHE_LOCK_FREE)
+mem_block_cache block_cache;

Hmm.... That's messy. A possibility is this:

Change the constructor of mem_block_cache to a member function, say...
initialize()

Add a std::once_flag to the mem_block_cache object.

In the get() and put() function use std::call_once to call initialize()

Assuming std::call_once / boost::call_once has a well implemented
fast-path, I think this should work and maintain performance.

That would work, and would only be needed for boost::atomic as
std::atomic can be statically initialized. Would have to use
boost::call_once though, which brings in a dependency on Boost.Thread
which I'd rather avoid if possible.

Since most current compilers have is it sufficient to just
switch between std::atomic or else the old mutex code and leave
boost::atomic out of it altogether?

This comment has been minimized.

@ylow

ylow Dec 17, 2015
Author Contributor

Yeah. that might be the simplest option for now. I will replace all boost::atomic with just std::atomic and static initialize that.

@ylow
Copy link
Contributor Author

@ylow ylow commented Dec 31, 2015

Sorry for the delay. I have updated the pull request with the fixes.

@jzmaddock
Copy link
Collaborator

@jzmaddock jzmaddock commented Jan 9, 2016

Sorry for the delay. I have updated the pull request with the fixes.

I'm also backlogged, will get to this shortly, John.

@jzmaddock jzmaddock merged commit d8af53e into boostorg:develop Jan 10, 2016
@jzmaddock
Copy link
Collaborator

@jzmaddock jzmaddock commented Jan 10, 2016

Many thanks for this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.