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

Add JMH support and improve MultiSpanExporter performance #678

Merged
merged 7 commits into from
Jan 31, 2020
Merged

Add JMH support and improve MultiSpanExporter performance #678

merged 7 commits into from
Jan 31, 2020

Conversation

sfriberg
Copy link
Contributor

This PR adds support for building benchmarks as part of opentelemetry using JMH. Simply add benchmarks/java/ folder under source and your benchmark and an artifact with subproject-benchmark.jar will be built with them. To run either use gradle benchmarkRun or java -jar XYZ-benchmark.jar if you have copied/downloaded all artifacts to the same directory.

I added a single microbenchmark and did an optimization to the MultiSpanExporter implementation to avoid allocation that happens when you are doing iteration of a unmodifiable collection, each entry returned by the iterator will be wrapped by an unmodifiable entry to ensure no modifications can be done.

Running on my laptop so not the most stable env...

Before
Benchmark                          (exporterCount)  (spanCount)   Mode  Cnt      Score      Error   Units
MultiSpanExporterBenchmark.export                1         1000  thrpt    5  85884.912 ± 3031.709  ops/ms
MultiSpanExporterBenchmark.export                3         1000  thrpt    5  68166.748 ±  985.945  ops/ms

After
Benchmark                          (exporterCount)  (spanCount)   Mode  Cnt       Score       Error   Units
MultiSpanExporterBenchmark.export                1         1000  thrpt    5  213964.678 ± 62705.536  ops/ms
MultiSpanExporterBenchmark.export                3         1000  thrpt    5  163793.449 ± 53344.890  ops/ms

If someone has a good idea on how to avoid the ErrorProne warnings when compiling the generated JMH code please let me know.

Staffan Friberg added 2 commits November 20, 2019 11:01
@codecov-io
Copy link

codecov-io commented Nov 20, 2019

Codecov Report

Merging #678 into master will decrease coverage by 0.09%.
The diff coverage is 90%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master     #678     +/-   ##
===========================================
- Coverage     78.61%   78.51%   -0.1%     
- Complexity      777      779      +2     
===========================================
  Files           101      101             
  Lines          2777     2788     +11     
  Branches        268      270      +2     
===========================================
+ Hits           2183     2189      +6     
- Misses          492      494      +2     
- Partials        102      105      +3
Impacted Files Coverage Δ Complexity Δ
...ntelemetry/sdk/metrics/AbstractMeasureBuilder.java 100% <ø> (ø) 3 <0> (ø) ⬇️
.../main/java/io/opentelemetry/context/NoopScope.java 0% <ø> (ø) 0 <0> (?)
...pentelemetry/sdk/metrics/AbstractGaugeBuilder.java 100% <ø> (ø) 3 <0> (ø) ⬇️
...telemetry/sdk/metrics/AbstractObserverBuilder.java 100% <ø> (ø) 3 <0> (ø) ⬇️
...ntelemetry/sdk/metrics/AbstractCounterBuilder.java 100% <ø> (ø) 3 <0> (ø) ⬇️
...io/opentelemetry/sdk/metrics/SdkDoubleMeasure.java 58.62% <100%> (ø) 3 <0> (ø) ⬇️
...io/opentelemetry/sdk/metrics/SdkDoubleCounter.java 58.62% <100%> (ø) 3 <0> (ø) ⬇️
...a/io/opentelemetry/sdk/metrics/SdkLongMeasure.java 58.62% <100%> (ø) 3 <0> (ø) ⬇️
...a/io/opentelemetry/sdk/metrics/SdkLongCounter.java 58.62% <100%> (ø) 3 <0> (ø) ⬇️
...java/io/opentelemetry/sdk/metrics/SdkLabelSet.java 85.71% <100%> (+7.14%) 7 <1> (ø) ⬇️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a05cca0...a3e4e62. Read the comment docs.

benchmark.gradle Outdated
enabled benchmarksAvailable
description 'Run benchmarks. Use property benchmarkFilter to set benchmark filtering regex.'
group 'Verification'
classpath = sourceSets.benchmark.runtimeClasspath
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think jmh works by default with jmh directory not benchmark, am I wrong?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure there is a 'default' directory. I opted for benchmark in similar fashion to 'test' which describes the purpose of the code in the sourceset, but not the test framework (junit, testng, ...).

No problem to change if there is a preference for jmh.

Working on getting the CLA signed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If jmh is the default, then that's the way I would go, as well.

@bogdandrutu
Copy link
Member

@sfriberg can you please sign the CLA and respond to the comment(s)?

@jkwatson
Copy link
Contributor

@sfriberg any update on the CLA?

@sfriberg
Copy link
Contributor Author

Got some issues with my CNCF account so have emailed them to see if it can be resolved so I can sign the CLA

@sfriberg
Copy link
Contributor Author

I signed it

Copy link
Contributor

@jkwatson jkwatson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is excellent. I'd love to get this merged, as I would like to start writing some benchmarks for various bits of the system ASAP.

@jkwatson
Copy link
Contributor

Before we merge this, can you rebase off of master and try running the benchmark? I think the SpanData creation in here will probably fail due to some changes in that class between when you wrote it and now.

Fix to work with latest master
@sfriberg
Copy link
Contributor Author

@jkwatson Thanks, yes had to do a minor change to make it work

Added some comments in the script about how to use it as well so I think it should be in a good state to merge now.

@jkwatson
Copy link
Contributor

:shipit:

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

Successfully merging this pull request may close these issues.

5 participants