1.x Remove all instances of Atomic*FieldUpdater #3488
Conversation
|
There is a reason for #3459 is Samsung's problem. Even though we see it in our apps (somebody told me that it's about 5% of Samsung devices of our users) I don't think that such platform specific bugs should be solved on library side. |
|
As @artem-zinnatullin mentioned, field updaters were introduced to reduce number of allocations and runtime size of the library; especially for the Android platform. Although field updaters have 1-5% overhead compared to Unsafe, the same overhead may manifest with Atomic instances due to the one level indirection, plus usually the surrounding logic forces a re-read of the AtomicXXX field all the time if not done with care. See my other comments in the code. |
| @SuppressWarnings("rawtypes") | ||
| static final AtomicReferenceFieldUpdater<LatestObserverIterator, Notification> REFERENCE_UPDATER | ||
| = AtomicReferenceFieldUpdater.newUpdater(LatestObserverIterator.class, Notification.class, "value"); | ||
| AtomicReference<Notification<? extends T>> value = new AtomicReference<Notification<? extends T>>(); |
|
I started a run on this branch now using |
|
I ran JMH yesterday for this commit and the preceding one. The results are on my computer at home, and I'm at work now. I'll post them when I get back |
|
Here is the screenshot. The first result is from 51527b7, the second one is from after the changes in this pull request: @akarnokd I'm not sure if I used the GUI right. I can't get it to use something as a baseline. I pasted the result from 51527b7 first, and then the result from this pull request. Do I need to do something else? |
|
Right click in a cell and you get a popup menu with options. |
|
Otherwise, no performance regression. 👍 |
|
👍 |
| @@ -139,7 +135,8 @@ public void request(long n) { | |||
| * that there is always once who acts on each `tick`. Same concept as used in OperationObserveOn. | |||
| */ | |||
| void tick() { | |||
| if (WIP.getAndIncrement(this) == 0) { | |||
| AtomicLong localCounter = this.counter; | |||
There was a problem hiding this comment.
Any reason for having copy of final reference?
|
👍, few nits. Looks like @akarnokd wants local copies of @akarnokd can you please give a link or describe how local copy of |
|
Keep the locals as I suggested. JIT re-reads them unfortunately. I read and experienced this myself with JCTools queries and range(). |
|
That's sad. Okay, then just SuppressWarnings need to be fixed. Netflix team: can we expect 1.0.16 soon? As mentioned in the original issue On Wed, Nov 11, 2015, 11:24 David Karnok notifications@github.com wrote:
@artem_zin |
|
@artem-zinnatullin I removed the erroneous annotations |
|
👍, thanks! |
|
Please squash this again. |
Replace them all with their respective Atomic* counterparts For example AtomicLongFieldUpdater -> AtomicLong Addresses ReactiveX#3459
|
Done |
1.x Remove all instances of Atomic*FieldUpdater
|
Thanks for contributing. |
Replace them all with their respective Atomic* counterparts
For example AtomicLongFieldUpdater -> AtomicLong
Addresses #3459