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 JDK's LongAdder #1055

Merged
merged 2 commits into from Feb 7, 2017

Conversation

Projects
None yet
3 participants
@arteam
Member

arteam commented Feb 2, 2017

Add an ability to use the JDK's implementation if it's available in runtime. Add a factory for creating LongAdders which tries to use the JDK's implementation and fallbacks to the internal one if the JDK doesn't provide any. Unfortunately, LongAdders don't have a common interface, therefore we introduce LongAdderAdapter. It will be used in other components as an interface, a concrete implementation of it will be created by LongAdderFactory.

@arteam

This comment has been minimized.

Show comment
Hide comment
@arteam

arteam Feb 2, 2017

Member

@ryantenney did something similar for the 4.0 branch in #876

Member

arteam commented Feb 2, 2017

@ryantenney did something similar for the 4.0 branch in #876

@arteam

This comment has been minimized.

Show comment
Hide comment
@arteam

arteam Feb 2, 2017

Member

This also should fix ThreadLocal leaks in the internal LongAdder implementation (by using the JDK implementation on a modern JRE) as reported in #375.

Member

arteam commented Feb 2, 2017

This also should fix ThreadLocal leaks in the internal LongAdder implementation (by using the JDK implementation on a modern JRE) as reported in #375.

@arteam arteam added this to the 3.1.3 milestone Feb 2, 2017

@arteam arteam added the improvement label Feb 2, 2017

@arteam

This comment has been minimized.

Show comment
Hide comment
@arteam

arteam Feb 2, 2017

Member

Bummer. The build can't compiled on Travis on JDK 7, because LongAdder doesn't exist in the JDK.

@dropwizard/dropwizard-metrics I'm wondering if we can remove it (JDK 6/7 will be still supported in runtime).

Member

arteam commented Feb 2, 2017

Bummer. The build can't compiled on Travis on JDK 7, because LongAdder doesn't exist in the JDK.

@dropwizard/dropwizard-metrics I'm wondering if we can remove it (JDK 6/7 will be still supported in runtime).

@ryantenney

This comment has been minimized.

Show comment
Hide comment
@ryantenney

ryantenney Feb 3, 2017

Member

You've got the right idea, but what I'd recommend is adding a metrics-java8 module that is compiled using Java 8 (and is ignored if the compiler is <8). Then if someone wants to use the native longadder, they can import that module.

I went looking for an old branch where I started to do just that, but I can't find it.

Member

ryantenney commented Feb 3, 2017

You've got the right idea, but what I'd recommend is adding a metrics-java8 module that is compiled using Java 8 (and is ignored if the compiler is <8). Then if someone wants to use the native longadder, they can import that module.

I went looking for an old branch where I started to do just that, but I can't find it.

@arteam

This comment has been minimized.

Show comment
Hide comment
@arteam

arteam Feb 4, 2017

Member

Thanks for the feedback! I got your point, but I think it would be better to implement this feature in a maintenance release without additional modules. The internal LongAdder is not only has worse performance, but it's prone to memory leaks (because of its use of thread locals). I think it would be great to provide a release which will just fix the issue without requiring the developer/administrator to enable an additional module (so the metrics jar could be just replaced in an application server).

Member

arteam commented Feb 4, 2017

Thanks for the feedback! I got your point, but I think it would be better to implement this feature in a maintenance release without additional modules. The internal LongAdder is not only has worse performance, but it's prone to memory leaks (because of its use of thread locals). I think it would be great to provide a release which will just fix the issue without requiring the developer/administrator to enable an additional module (so the metrics jar could be just replaced in an application server).

@arteam

This comment has been minimized.

Show comment
Hide comment
@arteam

arteam Feb 4, 2017

Member

I've updated the pull request to align the implementation with the ThreadLocal one from #1052

Member

arteam commented Feb 4, 2017

I've updated the pull request to align the implementation with the ThreadLocal one from #1052

arteam added some commits Feb 1, 2017

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.
@jplock

This comment has been minimized.

Show comment
Hide comment
@jplock

jplock Feb 7, 2017

Member

Looks good to me, want me to merge this in @ryantenney ?

Member

jplock commented Feb 7, 2017

Looks good to me, want me to merge this in @ryantenney ?

@ryantenney

This comment has been minimized.

Show comment
Hide comment
@ryantenney

ryantenney Feb 7, 2017

Member

@jplock go for it!

Member

ryantenney commented Feb 7, 2017

@jplock go for it!

@jplock jplock merged commit f1733fa into 3.1-maintenance Feb 7, 2017

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

@jplock jplock deleted the long_adder_delegate branch Feb 7, 2017

arteam added a commit that referenced this pull request Feb 15, 2017

Merge changes from the 3.1 branch to 3.2 (#1071)
* 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment