-
Notifications
You must be signed in to change notification settings - Fork 564
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
Flat Combining universal construct #313
base: master
Are you sure you want to change the base?
Conversation
The WFIStack is added to the set of experimental data structures. This data structure has, from experience, proven useful for implementing a number of other concurrency primitives.
- Remove `Thread.yield()` call in spin-wait loop. - Add padding around the `WFIStack.head` field. - Add a push/pop concurrent stress/sanity test for WFIStack.
if (operation == null) { | ||
throw new NullPointerException("The operation cannot be null."); | ||
} | ||
OpNode op = new OpNode(operation); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 comments:
- I often see combiners (see https://github.com/franz1981/java-puzzles/blob/combiner/src/main/java/red/hat/puzzles/combiner/OptimizedCombiner.java#L150) can use a thread local reusable node to save allocating in the hot path, do you think could worth doing it? Given that execution completion mark the end of the node lifecycle it should make possible to pool it
- I see we poll with FIFO policy, why not leaving the choice of using different data-structures (eg lifo/fifo) to store the ops?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could do pooling. An apply method that separates enqueueing and application in time via a Future can't rely on pooling, though. Those operations will have to be unpooled. Perhaps they need to be in a separate stack as well to prevent the pooled stack from growing too big. The lock could be turned from a boolean to an int, and include a sequence so we can periodically truncate the stack and avoid leaking.
Applying the operations in FIFO order is how the linearisable consistency is achieved. Would be easy to add an option to turn that off.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about the consistency... since concurrent blocking apply calls overlap in time, I think we can pick an arbitrary ordering of these overlapping operations, and still be considered linearisable. The operations were already racing, so there's no externally observable sequence of events that can disagree with applying the operations in LIFO order.
Pull Request Test Coverage Report for Build 692
💛 - Coveralls |
|
||
for (; ; ) { | ||
if (tryLock()) { | ||
Iterable<OpNode> nodes = combineAndApplyAllOperations(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although counter-intuitive I think that would be nice to have a version of combineAndApplyAllOperations
that allow to limit the amount of work to be performed (the simpler way in the max number of combined operations, but can think to other metrics, or by using a cooperative suspention of some form eg ControlledRunnable
?) to allow other(s) Threads to pick remaining jobs (if the combiner thread isn't alone).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking it twice probably the limit in the combined list of pending ops work only if the list is FIFO. Need to think about it better
lock.set(false); | ||
OpNode peek = ops.peek(); | ||
if (peek != null) { | ||
peek.unpark(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add a comment to explain how much is important to do this :)
This PR is using #312 as a base line. Work harder, @nitsanw 😛
This is not yet ready to merge, I think. But it is ready for discussing the API. I'm on the fence about using type parameters for exceptions. Perhaps there should be more methods and operation types with different signatures that would make Java 8 method references more easily align with common signature patterns seen in the Java collections framework. It's also possible, I think, to separate the enqueueing from the execution, and have an apply method that returns a Future.