Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support the JDK's ThreadLocalRandom #1052

Merged
merged 1 commit into from Feb 7, 2017

Conversation

@arteam
Copy link
Member

@arteam arteam commented Feb 1, 2017

Add a factory which provides a ThreadLocalRandom implementation depending on its availability in runtime. If the JDK provides ThreadLocalRandom, use it. Otherwise, fallback to the internal implementation.

@arteam arteam force-pushed the support_jdk_thread_local_random branch 4 times, most recently from 562aeff to 2f129eb Feb 1, 2017
@arteam
Copy link
Member Author

@arteam arteam commented Feb 1, 2017

I dug out an old dusty JRE6 build (Travis doesn't support it anymore), and the tests passed locally.

@arteam
Copy link
Member Author

@arteam arteam commented Feb 2, 2017

@dropwizard/dropwizard-metrics Could someone please review this?

@@ -95,7 +95,7 @@ public void update(long value, long timestamp) {
try {
final double itemWeight = weight(timestamp - startTime);
final WeightedSample sample = new WeightedSample(value, itemWeight);
final double priority = itemWeight / ThreadLocalRandom.current().nextDouble();
final double priority = itemWeight / ThreadLocalRandomFactory.current().nextDouble();

This comment has been minimized.

@arteam

arteam Feb 2, 2017
Author Member

Alternate approach is to switch to Java 7, but that would break Java 6 - compatibility, which I think it's not desirable for a maintenance release .

This comment has been minimized.

@ryantenney

ryantenney Feb 4, 2017
Member

Agreed, we could do that for 3.2

@arteam arteam force-pushed the support_jdk_thread_local_random branch from 2f129eb to 284ff93 Feb 2, 2017
@arteam arteam added this to the 3.1.3 milestone Feb 2, 2017
@arteam arteam added the improvement label Feb 2, 2017
@arteam arteam force-pushed the support_jdk_thread_local_random branch from 284ff93 to 7154ab4 Feb 3, 2017
@ryantenney ryantenney self-requested a review Feb 4, 2017
* By default it tries to use the JDK's implementation and fallbacks to the internal
* one if the JDK doesn't provide any.
*/
class ThreadLocalRandomFactory {

This comment has been minimized.

@ryantenney

ryantenney Feb 4, 2017
Member

I think this should be ThreadLocalRandomProxy instead.

This comment has been minimized.

@arteam

arteam Feb 4, 2017
Author Member

This is a also a good name.

@arteam arteam force-pushed the support_jdk_thread_local_random branch from 7154ab4 to 752d244 Feb 4, 2017
*/
private static class JdkDelegate {

private static Random get() {

This comment has been minimized.

@ryantenney

ryantenney Feb 4, 2017
Member

Lets rename this to current, just to be consistent.

This comment has been minimized.

@arteam

arteam Feb 4, 2017
Author Member

Done

@arteam arteam force-pushed the support_jdk_thread_local_random branch from 752d244 to 406d280 Feb 4, 2017
@ryantenney
Copy link
Member

@ryantenney ryantenney commented Feb 4, 2017

This looks great. The only thing I was thinking about was trying to avoid the branch per call by making the JdkDelegate an instance class, creating a new delegate class for our backfill ThreadLocalRandom, and an interface that defines just the current() method. Add a final static field INSTANCE and set it to whichever instance is supported. Then the ThreadLocalRandomProxy.current() method would just call INSTANCE.current().

Add a proxy which provides a ThreadLocalRandom implementation depending on its
availability in runtime. If the JDK provides ThreadLocalRandom, use it. Otherwise,
fallback to the internal implementation.
@arteam arteam force-pushed the support_jdk_thread_local_random branch from 406d280 to 4487e8b Feb 4, 2017
@ryantenney
Copy link
Member

@ryantenney ryantenney commented Feb 4, 2017

That's fantastic! Thank you! You've tested this in JDK 6? I'm curious because just by looking at it, I'm not sure whether NoClassDefFound error would be thrown during the instantiation of JdkProvider, or if it would be thrown while calling JdkProvider.current.

@arteam
Copy link
Member Author

@arteam arteam commented Feb 4, 2017

Yes, I've run the suite in JDK6. If I add a e.printStackTrace() call after in the catch block I get this in the console:

/home/artem/Downloads/openjdk-1.6.0-b30-linux-amd64-installer/jre/bin/java com.intellij.rt.execution.junit.JUnitStarter
java.lang.NoClassDefFoundError: java/util/concurrent/ThreadLocalRandom
	at com.codahale.metrics.ThreadLocalRandomProxy.getThreadLocalProvider(ThreadLocalRandomProxy.java:38)
	at com.codahale.metrics.ThreadLocalRandomProxy.<clinit>(ThreadLocalRandomProxy.java:35)
	at com.codahale.metrics.ExponentiallyDecayingReservoir.update(ExponentiallyDecayingReservoir.java:98)
	at com.codahale.metrics.ExponentiallyDecayingReservoir.update(ExponentiallyDecayingReservoir.java:83)
	at com.codahale.metrics.ExponentiallyDecayingReservoirTest.quantiliesShouldBeBasedOnWeights(ExponentiallyDecayingReservoirTest.java:160)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:622)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:47)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:44)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:271)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:70)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:50)
	at org.junit.runners.ParentRunner$3.run(ParentRunner.java:238)
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:63)
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:236)
	at org.junit.runners.ParentRunner.access$000(ParentRunner.java:53)
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:229)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:309)
	at org.junit.runners.Suite.runChild(Suite.java:127)
	at org.junit.runners.Suite.runChild(Suite.java:26)
	at org.junit.runners.ParentRunner$3.run(ParentRunner.java:238)
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:63)
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:236)
	at org.junit.runners.ParentRunner.access$000(ParentRunner.java:53)
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:229)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:309)
	at org.junit.runner.JUnitCore.run(JUnitCore.java:160)
	at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:68)
	at com.intellij.rt.execution.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:51)
	at com.intellij.rt.execution.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:237)
	at com.intellij.rt.execution.junit.JUnitStarter.main(JUnitStarter.java:70)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:622)
	at com.intellij.rt.execution.application.AppMain.main(AppMain.java:147)
Caused by: java.lang.ClassNotFoundException: java.util.concurrent.ThreadLocalRandom
	at java.net.URLClassLoader$1.run(URLClassLoader.java:217)
	at java.security.AccessController.doPrivileged(Native Method)
	at java.net.URLClassLoader.findClass(URLClassLoader.java:205)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:323)
	at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:294)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:268)
	... 40 more

I suppose the exception is thrown when the JdkProvider class is loaded by the JVM.

@arteam arteam mentioned this pull request Feb 4, 2017
@ryantenney
Copy link
Member

@ryantenney ryantenney commented Feb 7, 2017

Merge away!

@arteam arteam merged commit 2f12e86 into 3.1-maintenance Feb 7, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@arteam arteam deleted the support_jdk_thread_local_random branch Feb 7, 2017
@bobwenx
Copy link

@bobwenx bobwenx commented Feb 14, 2017

When is this fix release into maven repository?

@arteam
Copy link
Member Author

@arteam arteam commented Feb 14, 2017

I think we could make a release this week. Seems like nothing remains in the queue for 3.1.3.

arteam added a commit that referenced this pull request Feb 15, 2017
* Support for publishing metrics of BigInteger and BigDecimal type

* Update third-party.rst

New reporter - metrics-munin-reporter

* Add links to more third party libs

* Update third-party to include metrics-circonus

* Fix spelling in core.rst

* #814 always close socket

(cherry picked from commit 2762ce5)

* Revert "Add log4j2 xml-based config support"

This reverts commit 6667014.

* Revert "Added log4j2 InstrumentedAppender xml support to docs"

This reverts commit 26c4cd2.

* Revert "Fix javadoc link"

This reverts commit 28e6556.

* Revert "Add CPU profiling support to the AdminServlet (fixes #927)"

This reverts commit bbb52c1.

* Revert "Support for publishing metrics of BigInteger and BigDecimal type"

This reverts commit 7cfc23c.

* Suppress all kinds of Exceptions raised by report() (#1049) (#1056)

* Support the JDK's ThreadLocalRandom (#1052)

Add a proxy which provides a ThreadLocalRandom implementation depending on its
availability in runtime. If the JDK provides ThreadLocalRandom, use it. Otherwise,
fallback to the internal implementation.

* Support JDK's LongAdder (#1055)

* Support the JDK's LongAdder

Add an ability to use the JDK's implementation if it's available in the
runtime. Use a proxy for creating LongAdders which tries to use the JDK's
implementation and fallbacks to the internal one if the JDK doesn't
provide any.

Unfortunatelly, `LongAdder`s don't have a common interface, therefore we
introduce `LongAdderAdapter` as a common interface for `LongAdder`s. It
will be used in other components as an interface, but a concrete
implementation of it will be created by `LongAdderProxy`.

This pattern allows to us to still run code in the JRE, but enjoy the
benefits of the stock `LongAdder` in modern JDKs (less bugs, better
performance).

* Run tests only on JDK8

We need a compile dependency on LongAdders which is not available
in JDK7.

* Retry DNS lookups (#1064)

Do retry DNS lookups, which are just done in the constructor of
InetSocketAddress.

Reason might be that in some environments DNS is very dynamic and thus a name
might be sometimes resolveable and sometimes not. The way this is currently
implemented the InetSocketAddress is always cached, which in itself caches the
return value of the last DNS lookup, even if that failed.

* Backport rescale bug (#1046)

* Added ExponentiallyDecayingReservoir test illustrating the rescale race condition.

(cherry picked from commit d9af3c1)

* Fixed the race condition in ExponentiallyDecayingReservoir's rescale method.

(cherry picked from commit b022f04)
@fabienbancharel
Copy link

@fabienbancharel fabienbancharel commented May 11, 2017

Ryan, as you wondered : "I'm not sure whether NoClassDefFound error would be thrown during the instantiation of JdkProvider, or if it would be thrown while calling JdkProvider.current", in fact, testing 3.1.4, on Weblogic 10.3 and Oracle JDK 6, the exception is thrown only when JdkProvider.current is called, not when JdkProvider constructor is called, so 3.1.4 is KO. Curiously, on Tomcat 7 and the same Oracle JDK 6, the exception is thrown when the JdkProvider constructor is called.

I made a pull request for my tested workaround (#1129).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.