AbstractCache.SimpleStatsCounter LongAddr usage of Striped64 breaks on Android

[GoogleCodeExporter]

I'm using CacheBuilder in an Android app and when I use 
CacheBuilder.recordStats() it fails because SimpleStatsCounter uses Striped64, 
which uses Unsafe, which isn't present on Android (or at least doesn't work in 
the same way).

Ideally the API would allow me to *easily* provide an alternatice 
implementation, but I currently have to do some OO gymnastics to make it work 
(or not use stats at all). For example recordStats(StatsSupplier) would fit 
into the current builder API style.

Original issue reported on code.google.com by alex.rad...@gmail.com on 13 Dec 2012 at 10:12

[GoogleCodeExporter]

Do you have data on what version of Android you're building for, and what part 
of sun.misc.Unsafe is different or unsupported on Android?

I think the "right solution" is to make this work out of the box on Android, 
not to expose any additional API.

Original comment by lowas...@google.com on 13 Dec 2012 at 10:20

• Added labels: Type-Defect
• Removed labels: Type-Enhancement

[GoogleCodeExporter]

I'm developing on Android 4.2, this all started out because ProGuard complains 
about the Striped64 usage of Unsafe.compareAndSwapLong() and 
Unsafe.compareAndSwapInt(). I'll be honest and say I don't know if Unsafe is 
available at runtime as it is stripped out of my classes.dex due to the 
aforementioned ProGuard complaints.

I'm guessing it's using Unsafe rather than AtomicInt/Long for some extra 
performance gain. I was contemplating rewriting Striped64 using AtomicX in the 
short term or using a more vanilla version using primitive longs.

Original comment by alex.rad...@gmail.com on 13 Dec 2012 at 10:28

[GoogleCodeExporter]

Actually rewriting Striped64 with Atomics looks pretty tough... Maybe a simple 
stats counter using Atomics would suffice for both desktop and Android use 
cases? I'll leave it in your capable hands.

Original comment by alex.rad...@gmail.com on 13 Dec 2012 at 10:33

[GoogleCodeExporter]

After a quick Google search, 
http://www.java2s.com/Open-Source/Android/android-core/platform-libcore/sun/misc
/Unsafe.java.htm seems to suggest at least some version of Android has 
Unsafe.cAS{Long,Int}.

Have you tried building your app without ProGuard?

Original comment by lowas...@google.com on 13 Dec 2012 at 10:35

[GoogleCodeExporter]

(FYI, we use Caches in production here at Google, so I suspect we would want an 
implementation that performs as well as the current one.)

Original comment by lowas...@google.com on 13 Dec 2012 at 10:37

[GoogleCodeExporter]

At the moment I need to use ProGuard because I'm actually developing in Scala 
which is unusable without it. It may be as simple as creating a stub version of 
the Unsafe class for use a build time so PG can at least resolve it and then it 
should *just work* when deployed onto the device. I'll give that a try and see 
how I go.

Original comment by alex.rad...@gmail.com on 13 Dec 2012 at 10:39

[GoogleCodeExporter]

Actually part of the problem is here in Striped64:
 java.lang.reflect.Field f = sun.misc.Unsafe.class.getDeclaredField("theUnsafe");
On android the field is called "THE_ONE" so it will never resolve.

Original comment by alex.rad...@gmail.com on 13 Dec 2012 at 10:50

[GoogleCodeExporter]

Hmmmkay.  Will take a look.

I suspect that in the worst-case scenario, we might set it up to try getting 
the Unsafe instance, and upon failure fall back to a simple Atomic 
implementation.  (This wouldn't be the first place where Guava had a "use 
Unsafe if possible, otherwise fall back to this traditional implementation" 
algorithm.(

Original comment by lowas...@google.com on 13 Dec 2012 at 11:05

[GoogleCodeExporter]

For Joe Average (ie me) a version of the stats counter based on Atomics would 
more than suffice :) I'm actually interested in knowing how big the performance 
different is, I'll add it to my never ending stack of cool things to do in my 
*spare* time.

Original comment by alex.rad...@gmail.com on 13 Dec 2012 at 11:07

[GoogleCodeExporter]

Some good news. I can confirm that if I hack Striped64 to use "THE_ONE" it now 
works. A fix could be as simple as just detecting Android in the environment as 
using the alternate string.

Original comment by alex.rad...@gmail.com on 13 Dec 2012 at 11:13

[GoogleCodeExporter]

By works I mean the original exception is not thrown.. I'm now debugging 
actually runtime behaviour. Will take a bit longer.

Original comment by alex.rad...@gmail.com on 13 Dec 2012 at 11:13

[GoogleCodeExporter]

It's essential to use "-dontwarn sun.misc.Unsafe" whenever building with 
ProGuard.  See 
https://code.google.com/p/guava-libraries/wiki/UsingProGuardWithGuava.  If you 
do that, does any problem remain?

Original comment by kevinb@google.com on 14 Dec 2012 at 5:00

[GoogleCodeExporter]

Whoops, just saw comment 10. We should probably fall back on looking for 
THE_ONE. And this goes for our other uses of Unsafe as well.  Also, I'm not 
sure we ever actually wrote a benchmark proving that this Striped64 stuff was 
really worth it in the first place.

Original comment by kevinb@google.com on 14 Dec 2012 at 5:04

• Changed state: Accepted

[GoogleCodeExporter]

All works as expected now. I created a stub sun.misc.Unsafe so I could hack 
Striped64 so I don't need -dontwarn anymore. If I find time I might write a 
microbenchmark to see if atomics would cut the mustard ;)

Original comment by a...@radeski.net on 14 Dec 2012 at 10:18

[GoogleCodeExporter]

As promised a micro benchmark... https://github.com/al3ks/cachestatsmicrobench

Unless I stuffed my test up, it's orders of magnitude better with more threads 
as per its design spec. So my guess it's worth keeping ;)

Striped64Bench A - Threads=8 - Duration=1.1620s
AtomicBench A - Threads=8 - Duration=14.3500s
Striped64Bench B - Threads=8 - Duration=1.1430s
AtomicBench B - Threads=8 - Duration=15.3570s

Original comment by alex.rad...@gmail.com on 15 Dec 2012 at 10:46

[GoogleCodeExporter]

Actually implemented a thread-local striped long that appears to perform even 
better than Striped64. I was trying to mimic Striped64 behaviour without using 
sun.misc.Unsafe and to my surprise it actually performs better under heavy 
load. It's still available here if you want to have a look: 
https://github.com/al3ks/cachestatsmicrobench

Original comment by alex.rad...@gmail.com on 15 Dec 2012 at 12:22

[GoogleCodeExporter]

@Alex: 
- Why does the thread computing `LocalStripedLongAdder.sum()` see the changes 
made by the others? AFAIK making `LongRef.value` volatile would guarantee it, 
but also cause a slowdown by factor 2 (still the fastest in my benchmark).
- Each instance of `LocalStripedLongAdder` needs its own `ThreadLocal`, this 
might be costly (or not; this should be analyzed).
- You're measuring the speed of `CacheStats` in isolation. Getting a 10x speed 
ratio here may have a similar impact on the cache speed. Or none at all.
- I'd suggest using http://code.google.com/p/caliper; see also the tips there.

Original comment by Maaarti...@gmail.com on 15 Dec 2012 at 9:00

[GoogleCodeExporter]

@Maarti: Wasn't aware of Caliper... I'll take a look.
- No need to make it volatile, as I keep an array of all the instances of 
objects created per thread in the 'adders' field. As this is a 
CopyOnWriteArrayList, it can safely be used from any thread to grab a snapshot 
of the thread-local counters.
- I don't believe the ThreadLocal object cost is high, worth a look but would 
be surprised if it's a show stopper.
- I am focusing on CacheStats only, because actually this didn't start out as 
me trying to improve performance, I was trying to move away from 
sun.misc.Unsafe while not significantly losing performance. It just so happens 
it went significantly faster and is much simpler code. I don't really expect 
this to improve Cache performance overall, just not to do any harm when stats 
are enabled.

Original comment by a...@radeski.net on 15 Dec 2012 at 9:59

[GoogleCodeExporter]

Ah I see what you mean now about LongRef.value and volatile... interesting.

Original comment by a...@radeski.net on 15 Dec 2012 at 10:05

[GoogleCodeExporter]

Not sure when you had a look on github but I've added two more version one with 
a StripedLock and another with a StripedAtomic. Both perform to a similar level 
as the StripedLocal, although the StripedAtomic is the most thread-safe and 
consistent.

The StripedLocal and StripedLock implementations only offer quiescent 
consistency over strict consistency. This probably isn't acceptable for Guava...

Original comment by a...@radeski.net on 15 Dec 2012 at 10:19

[GoogleCodeExporter]

Personally, I feel fairly comfortable trusting that Doug Lea's implementation 
of LongAdder is going to be pretty effective, though I will do some 
benchmarking myself.

In any event, I submitted a fix to make it work with sun.misc.Unsafe on 
Android, as well as providing a fallback if sun.misc.Unsafe is unavailable, 
which would apply IIRC on e.g. AppEngine.  The fix should be mirrored out 
externally in the next few days.

Original comment by wasserman.louis on 15 Dec 2012 at 10:59

• Changed state: Fixed

[GoogleCodeExporter]

You're right, I have fixed up my micro benchmark and Doug's impl is the winner, 
although my StripedLocal comes in close with much less code. In any case, 
thanks for fixing the main issue.

Original comment by a...@radeski.net on 15 Dec 2012 at 11:10

[GoogleCodeExporter]

Original comment by lowas...@google.com on 26 Dec 2012 at 5:03

• Added labels: Milestone-Release14

[GoogleCodeExporter]

We are also using Guava for Android (we being The Weather Channel) and I just 
turned on CacheStats only to discover the same problem with Striped64.  
Unfortunately, getting the fix is not an option for us because we are stuck on 
this jdk5 version of Guava 13.0 (see link) due to the fact that Android 8 
doesn't have NavigableSet (another long story).

http://mvnrepository.com/artifact/com.google.guava/guava-jdk5

So, can I please get a bit of advice?  I would really love to be able to get 
some stats on our Guava caches.  Is there a way to do that that's not too 
difficult without waiting for the fix mentioned above?

Thanks!

Original comment by nachobur...@gmail.com on 2 Jan 2013 at 1:56

[GoogleCodeExporter]

Well, Guava 14 will also get a 1.5 backport. Will upgrading to that when it's 
out (soon!) be an option?

It may have been a mistake that we backported the Striped64 stuff at all; might 
be better overall to roll back that change in the backport branch. Just a 
suggestion, which Louis will have better judgment about.

I love weather.com, you guys are awesome. Though I have to say I was out 
sledding here in Tahoe yesterday and just five more degrees or so would really 
have been appreciated.

Original comment by kevinb@google.com on 4 Jan 2013 at 8:29

[GoogleCodeExporter]

If there is no other option other than to wait for the Guava 14 backport then I 
suppose that's all we can do.  How soon is soon?  :)

If I had my way we would abandon Android API 8, but we still have enough 
customers with older devices (barely) to make that not an option.

We have a completely revamped version of our app which is supposed to be code 
complete soon (there's that word again) and I would feel all warm and fuzzy if 
I could see the cache stats before it goes live, but you know what Mick Jagger 
says about getting stuff you want.

We love Guava.  It's been a year or two since we integrated it into our product 
and it's been great.  Especially the caching - which we use like crazy.  Thanks!

Original comment by nachobur...@gmail.com on 4 Jan 2013 at 11:26

[GoogleCodeExporter]

This issue has been migrated to GitHub.

It can be found at https://github.com/google/guava/issues/<id>

Original comment by cgdecker@google.com on 1 Nov 2014 at 4:13

• Added labels: MigratedToGitHub

[GoogleCodeExporter]

Original comment by cgdecker@google.com on 3 Nov 2014 at 9:08

• Added labels: Restrict-AddIssueComment-Commit