There is a reason for AtomicFieldUpdater + volatile value — lower memory consumption (cc @akarnokd), but personally, I'd like to switch to Atomic fields too to get rid of reflection in such hot code.

#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.

I started a run on this branch now using ./gradlew benchmarks '-Pjmh=.*OperatorObserveOnPerf.*'. The ETA for that is in over 2 hours. Does that seem right?

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. 👍

👍

👍, few nits. Looks like @akarnokd wants local copies of final references, but I don't, though I'm ok with having them — you can just fix @SuppressWarnings("unused").

@akarnokd can you please give a link or describe how local copy of final reference will help? I mean, JIT will ~100% won't make re-reads of final references until it detect change of the reference via reflection. Or I'm wrong?

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
this PR would prevent crashes in many apps.

On Wed, Nov 11, 2015, 11:24 David Karnok notifications@github.com wrote:

Keep the locals as I suggested. JIT re-reads them unfortunately. I read
and experienced this myself with JCTools queries and range().

—
Reply to this email directly or view it on GitHub
#3488 (comment).

@artem_zin

@artem-zinnatullin I removed the erroneous annotations

👍, thanks!

Please squash this again.

Done

Thanks for contributing.