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

Use optimistic behavior in Barrier #952

Merged
merged 4 commits into from
May 23, 2016
Merged

Use optimistic behavior in Barrier #952

merged 4 commits into from
May 23, 2016

Conversation

codemercenary
Copy link
Contributor

This eliminates one lock acquisition during Barrier and implements a double-check pattern, which makes the Barrier routine significantly faster in cases where the queue is empty.

This eliminates one lock acquisition during Barrier and implements a double-check pattern, which makes the Barrier routine significantly faster in cases where the queue is empty.
@wmisha
Copy link
Contributor

wmisha commented May 21, 2016

Makes sense to me.

Shouldn't the if (onAborted) and both of the if (!m_count) checks be added to the non-timed Barrier() as well?

@codemercenary
Copy link
Contributor Author

Actually that's a good point. The difference in behavior is going to have to be fixed another way.

In fact, if the count is zero, I still want to block for a single element from the queue to be dispatched. The reason is that the Barrier method is sometimes used to wait for a thread to finish starting, which means that I can't actually optimistically return just on the basis of the value of m_count, unless the timeout is also zero.

@wmisha
Copy link
Contributor

wmisha commented May 23, 2016

Doesn't it still save computation if you keep the check on line 297?

And with this new logic, if timeout is zero but count is not, then shouldn't it still fail?

@codemercenary
Copy link
Contributor Author

Ah crap, yes that was the intention, I wrote it wrong.

@codemercenary
Copy link
Contributor Author

Also I can't keep that check. For the function to behave properly, I need to always pend that lambda to the queue and wait for the main thread to dispatch it, otherwise the timed Barrier routine cannot be used to wait for threads to start.

@wmisha
Copy link
Contributor

wmisha commented May 23, 2016

Does the caller of Barrier know it's waiting for the thread to start? I imagine that that use case could be handled by the untimed Barrier instead, since the behavior is effectively untimed.

Unless "timed" in this case means:

  • If the thread has started, wait for at most this length of time
  • Otherwise, wait arbitrarily long for the thread to start

@codemercenary
Copy link
Contributor Author

This class is used as the base class for CoreThread, and CoreThread calls WaitForEvent to drive the dispatch queue.

In Autowiring, I try to use the following paradigm:

  1. All wait operations return void. Error conditions are signified with exceptions.
  2. All wait operations have a timed variant that returns bool that signifies whether a timeout happened.

This is meant to parallel std::condition_variable::wait and std::condition_variable::wait_for, which have the same design.

The untimed DispatchQueue::Barrier would block until the next call to DispatchQueue::DispatchEvent. So, I need to ensure the timed variant of Barrier exhibits identical behavior.

An general exception are cases where zero is specified for the timeout. In that case, we infer that the user does not actually want to wait for anything, and instead the call should be interpreted to be a check for the underlying condition rather than a desire to block for some condition to be true.

@wmisha
Copy link
Contributor

wmisha commented May 23, 2016

Ok, that makes more sense. Thanks for the explanation.

@wmisha wmisha merged commit 3a64e9f into master May 23, 2016
@wmisha wmisha deleted the feature-optimistic branch May 23, 2016 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants