Moved to atomic field updaters. #1246
Conversation
|
RxJava-pull-requests #1146 FAILURE |
|
I've been digging into the failure case and it seems something in the Clojure wrapper calls the merger's Subscriber.onCompleted multiple times which is a violation of the observer contract. The call comes from chunk.clj:68. |
|
This looks like a great set of changes ... however, huge amounts of code have been touched so it's making it really hard to review this. Most of the changes seem to be formatting changes, is that the case? If so, can you prevent that so as to simplify the review? This is 1800 lines of code to review :-) |
|
Whenever I use fix-indent in Netbeans, this is the result, few spaces added here, few spaces removed there. But only a fraction is just formatting changes. |
|
Adding |
|
@vinc3m1 |
nice hidden feature
| return buf.take(); | ||
| } | ||
|
|
||
| void setWaiting(int value) { | ||
| WAITING_UPDATER.lazySet(this, value); |
There was a problem hiding this comment.
Help me understand why lazySet is safe to use here. It seems we want to have visibility of this value when we next read, but my understanding of lazySet is that it does not guarantee that.
There was a problem hiding this comment.
I read up further on lazySet and added another comment to code above.
There was a problem hiding this comment.
The value set here is eventually visible, which may take a few nanoseconds. The benefit is that the lazySet doesn't flush the store buffer whereas a volatile write does, which incurs 10-40 nanosecond cache-coherency traffic. Generally, the lazySet only makes sure writes that happened before are not reordered after the lazy write; which is generally enough in a SPSC value transmission case.
There was a problem hiding this comment.
Why is eventually visible safe here? It doesn't seem to be.
There was a problem hiding this comment.
My main guide was a blog you recommended:
http://psy-lob-saw.blogspot.co.uk/2012/12/atomiclazyset-is-performance-win-for.html
If you wish, I can eliminate all lazySet calls so there won't be any visibility concerns.
There was a problem hiding this comment.
I'm trying to understand when it's safe. I get the use case for setting something null in cleanup, I'm more concerned in places where we set it and need to read it without any further writes guaranteed to happen before.
|
What do you think needs to be changed before merging? Do any of the |
|
Ah I see you just reverted the use of |
|
RxJava-pull-requests #1154 FAILURE |
|
Let's be on the safe side for now; the fact that it works on x86 doesn't mean it works properly on other platforms. |
|
Hmm, we need to figure out that clojure test failure. @daveray Can you help us understand what's going on? It's not obvious why anything this PR changes should affect a single test in Clojure code. |
|
What I could understand is that the chunk_test.clj creates an Observable which invokes onNext after a random delay. It seems this observable emits onCompleted several times. The reason this fails now because merge() is changed to match the other mergeXYZ behavior by counting down a wip in both the outer and inner subscribers. For normal behaving sources, it makes sure the last one to reach wip = 0 will call onCompleted on the actual. The previous merge() didn't do this; it set a completed flag and checked for wip = 0; I suspect this oddity came from this problematic test and its misbehaving observable. |
|
I can revert the merge() behavior to make that test work again and allow merging this PR, but we need to fix that test and have merge() implement the correct onCompleted behavior. |
|
RxJava-pull-requests #1155 SUCCESS |
|
Thanks for reverting that change. Submit that as a separate request. |
Moved to atomic field updaters.
Replaced the AtomicBoolean, AtomicInteger, AtomicLong and AtomicReference instances with volatile fields and field updaters to save on memory and allocation.
Unfortunately, the Clojure test
chunk_test.clj:test-chunkfails and I don't know why since I don't understand what it is supposed to do. It appears numbers get lost somehow.