flogger icon indicating copy to clipboard operation
flogger copied to clipboard

ThreadlocalRandom instead of ThreadLocal

Open MartinWitt opened this issue 1 year ago • 8 comments

Hey,

While reading the changes in the newest releases and the code for it, I saw this comment. https://github.com/google/flogger/blob/master/api/src/main/java/com/google/common/flogger/SamplingRateLimiter.java#L55 If I understand the build system correct flogger requires at least java 8, so ThreadLocalRandom(https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/ThreadLocalRandom.html) or ThreadLocal.withInitial is available. Is the comment a relict of the past and the code could be changed?

MartinWitt avatar Oct 19 '23 18:10 MartinWitt

Flogger is JDK 7 currently. https://github.com/google/flogger/blob/master/api/BUILD#L42 which is exactly why I had to avoid using ThreadLocalRandom when I wrote that code.

hagbard avatar Oct 20 '23 01:10 hagbard

But isn't ThreadLocalRandom a java 7 feature? https://docs.oracle.com/javase%2F7%2Fdocs%2Fapi%2F%2F/java/util/concurrent/ThreadLocalRandom.html

MartinWitt avatar Oct 20 '23 12:10 MartinWitt

Weirdly not, despite the initial JavaDoc saying so (many methods are individually listed as 1.8).

https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/ThreadLocalRandom.html

Basically I originally wrote it with TLR and it didn't compile, so I switched to doing it manually.

If someone can get it working (including Android) with TLR I'm fine with it, but it didn't work when I tried it.

I don't think it's a big deal though, but thanks for suggesting it.

David

hagbard avatar Oct 20 '23 13:10 hagbard

I'm also seeing that ThreadLocalRandom was added in 1.7:

  • https://github.com/openjdk-mirror/jdk7u-jdk/blob/master/src/share/classes/java/util/concurrent/ThreadLocalRandom.java
  • https://www.baeldung.com/java-thread-local-random
  • https://stackoverflow.com/questions/7139525/java-7-threadlocalrandom-generating-the-same-random-numbers
  • https://www.reddit.com/r/java/comments/108wia/avoid_random_use_new_threadlocalrandom_from_java7/
  • https://www.tutorialspoint.com/java_concurrency/concurrency_threadlocalrandom.htm

Not a huge deal, but probably an easy enough change to make if we wanted...

kluever avatar Oct 20 '23 13:10 kluever

The limitation is very likely Android.

cpovirk avatar Oct 20 '23 13:10 cpovirk

I guess there's a chance I started writing this bit of API before Flogger bumped to 1.7 and I'm misremembering which version was causing the issue. It's been internally available in Google for a while now.

hagbard avatar Oct 20 '23 13:10 hagbard

I can provide a PR refactoring this, if this doesn't create more work for you. Is there some android CI?

MartinWitt avatar Oct 23 '23 10:10 MartinWitt

The Android work is internal to Google and not open sourced.

hagbard avatar Oct 23 '23 10:10 hagbard