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

Pessimisation fix for condition_variable notify_* #1461

Merged
merged 8 commits into from Dec 15, 2018

Conversation

Projects
4 participants
@termoose
Copy link
Contributor

commented Dec 15, 2018

This should fix #1284. Some of these locks are for setting stopped and started flags, those functions could get a cleaner look by using std::atomic_bool on those flags and avoiding the explicit lock/unlock.

@termoose termoose referenced this pull request Dec 15, 2018

Closed

Avoid lock pessimisation #1284

@cryptocode
Copy link
Collaborator

left a comment

LGTM

@clemahieu

This comment has been minimized.

Copy link
Collaborator

commented Dec 15, 2018

I was trying to apply lock pessimisation avoidance before but in some case it seemed to create a race condition though how that would happen escapes me now.

As long as all shared variables that are modified are protected by the same mutex it should be safe to notify the condition outside the lock right?

Does this pass with a tsan run?

@cryptocode

This comment has been minimized.

Copy link
Collaborator

commented Dec 15, 2018

Should be safe and beneficial: "The notifying thread does not need to hold the lock on the same mutex as the one held by the waiting thread(s); in fact doing so is a pessimization, since the notified thread would immediately block again, waiting for the notifying thread to release the lock."

@termoose

This comment has been minimized.

Copy link
Contributor Author

commented Dec 15, 2018

Yes this is the way to go and should increase performance in some cases. It has been discussed here.

Unfortunately I'm not able to run Clang's ThreadSanitizer here on my Mac as it's not supported for some reason.

What do you think about using std::atomic_bools for those flags? Should that be a separate issue? I'd be happy to look into it. Will for example turn code like this from node.cpp:

  std::unique_lock<std::mutex> lock (mutex);
  started = true;

  lock.unlock ();
  condition.notify_all ();
  lock.lock ();

into this

  started.store(true); // can use std::atomic::load to make it explicit that we're dealing with atomics
  condition.notify_all ();
  std::unique_lock<std::mutex> lock (mutex);
@clemahieu

This comment has been minimized.

Copy link
Collaborator

commented Dec 15, 2018

Since the condition variable is tied to the mutex we can't avoid holding a mutex over setting the variable, for instance

T1 Check started (false)
T2 Set started (true)
T2 Notify listeners
T1 Wait on condition variable (stalled)

@clemahieu clemahieu merged commit 1a3ce38 into nanocurrency:master Dec 15, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@clemahieu

This comment has been minimized.

Copy link
Collaborator

commented Dec 15, 2018

Looks great, thanks!

@rkeene rkeene added this to the V18.0 milestone Dec 16, 2018

@zhyatt zhyatt added this to Unscheduled in V18 Dec 27, 2018

@cryptocode cryptocode moved this from Unscheduled to CP 1 (2018-01-09) in V18 Dec 28, 2018

@cryptocode cryptocode moved this from CP 1 (2018-01-09) to CP 0 in V18 Dec 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.