MINOR: Update jmh to 1.27 for async profiler support by ijuma · Pull Request #9129 · apache/kafka

ijuma · 2020-08-05T13:57:55Z

Also updated the jmh readme to make it easier for new people to know
what's possible and best practices.

There were some changes in the generated benchmarking code that
required adjusting spotbugs-exclude.xml and for a javac warning
to be suppressed for the benchmarking module. I took the chance
to make the spotbugs exclusion mode maintainable via a regex
pattern.

Tested the commands on Linux and macOS with zsh.

JMH highlights:

async-profiler integration. Can be used with -prof async,
pass -prof async:help to look for the accepted options.
perf c2c [2] integration. Can be used with -prof perfc2c,
if available.
JFR profiler integration. Can be used with -prof jfr, pass
-prof jfr:help to look for the accepted options.

Full details:

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

ijuma · 2020-08-05T14:19:49Z

Actually, it looks like I need to improve the jmh script to make this easier to use. Will also update the README. So, please don't review it yet.

ijuma · 2020-08-11T09:17:56Z

@omkreddy this is ready for review.

bbejeck

Thanks, @ijuma LGTM.

The test failures are related due to spotbugs findings. But considering what's changed here I can't see what it could be.

lbradstreet · 2020-08-11T15:13:07Z

Rather than "in order to verify that they are valid", how about "in order to verify that they are measuring what you think they are. For example, if you use mocks they may use reflection that can easily dominate the benchmark's run time.".

lbradstreet · 2020-08-11T15:17:20Z

While we're at it we can also mention -prof gc and how you should look for the norm allocation rate (since we care about garbage generated per operation rather than per second).

I'll provide some text for this soon.

Sure. Do you think I should have only the link to jmh-benchmarks/README.md and have all the information on to use it in that page?

Including only the link sounds good.

Let's drop this and include that link before merge.

Yeah, I fixed this in my local copy, but I wanted to test it before pushing the changes.

lbradstreet · 2020-08-22T16:22:48Z

@ijuma the text mode output here looks good however it would be good to include instructions for producing an svg flamegraph if possible. Do you know if we can do that?

lbradstreet

LGTM with a few suggested changes.

lbradstreet · 2020-08-27T19:08:30Z

Suggested addition:

Using JMH GC profiler

It's good practice to run your benchmark with -prof gc to measure the allocation rate for your code:

./jmh-benchmarks/jmh.sh -prof gc

Of particular importance is the "norm" alloc rates, which measure the allocations per operation rather than allocations per second which can increase when you have made your code faster.

lbradstreet · 2020-08-27T19:10:18Z

We should document this variant instead so linux and mac os users are covered.

-prof async:libPath=/Users/lucas/Downloads/async-profiler-1.8-macos-x64/build/libasyncProfiler.so

Agreed, I tested this variant on Mac, but not Linux yet. Will do before changing.

lbradstreet · 2020-10-11T18:10:27Z

@ijuma we should bump this to 1.26 as it has some relevant bug fixes, e.g. openjdk/jmh@2398e8e

chia7712

I love those new document about JMH usage.

ijuma · 2020-12-10T06:08:07Z

@lbradstreet @chia7712 @omkreddy Please take a look when you have a chance.

chia7712

@ijuma +1 to this nice documentation.

chia7712 · 2020-12-10T06:42:46Z

The file name of async-profiler is libasyncProfiler.so. Should we follow the naming in this example?

Yeah, that makes sense. Will do when I'm near a computer.

Highlights: * async-profiler integration. Can be used with -prof async, pass -prof async:help to look for the accepted options. * perf c2c [2] integration. Can be used with -prof perfc2c, if available. Full details: https://mail.openjdk.java.net/pipermail/jmh-dev/2020-August/002982.html

… code

ijuma · 2020-12-10T13:49:13Z

-            <Package name="org.apache.kafka.jmh.consumer.generated"/>
-        </Or>
+        <!-- Suppress some minor warnings about machine-generated code for benchmarking. -->
+        <Package name="~org\.apache\.kafka\.jmh\..*\.jmh_generated"/>


jmh now uses jmh_generated instead of generated.

ijuma · 2020-12-10T13:49:50Z

@chia7712 Pushed a couple of follow ups. I think this is ready to merge.

chia7712

+1 and thanks for this patch. It does give a good guideline about JMH on kafka.

lbradstreet · 2020-12-10T17:31:10Z

+
+### Using JMH with async profiler
+
+It's good practice to check profiler output for microbenchmarks in order to verify that they are valid.


How about:

"It's good practice to check profiler output for microbenchmarks in order to verify that they represent application behavior and measure what you expect to measure. Some example pitfalls include the use of expensive mocks or accidental inclusion of test setup code in the benchmarked code."

lbradstreet · 2020-12-10T17:32:58Z

+
+    java -jar <kafka-repo-dir>/jmh-benchmarks/build/libs/kafka-jmh-benchmarks-all.jar -f2 LRUCacheBenchmark
+
+### Writing benchmarks


Could we include a short section here about what should be put into a PR that has been benchmarked?

I'm thinking:

Benchmark comparisons for the code before and after the change.

-prof gc results.

An example async profile from at least one run.

Good idea. We should also link from the contributor docs.

I will do this in a separate PR since it's a bit unrelated to the original goal of this PR and it may take a few iterations to get right.

ijuma requested a review from omkreddy August 5, 2020 13:58

ijuma force-pushed the jmh-1.24 branch from 0a2eae1 to 7ec327b Compare August 5, 2020 14:12

ijuma force-pushed the jmh-1.24 branch from 455835d to debff25 Compare August 11, 2020 09:18

bbejeck approved these changes Aug 11, 2020

View reviewed changes

lbradstreet reviewed Aug 11, 2020

View reviewed changes

ijuma changed the title ~~MINOR: Update jmh to 1.24 for async profiler support~~ MINOR: Update jmh to 1.25 for async profiler support Aug 22, 2020

ijuma force-pushed the jmh-1.24 branch from 4a43805 to ad5b568 Compare August 22, 2020 23:09

lbradstreet approved these changes Aug 27, 2020

View reviewed changes

lbradstreet reviewed Aug 27, 2020

View reviewed changes

chia7712 approved these changes Oct 25, 2020

View reviewed changes

ijuma changed the title ~~MINOR: Update jmh to 1.25 for async profiler support~~ MINOR: Update jmh to 1.26 for async profiler support Dec 9, 2020

ijuma changed the title ~~MINOR: Update jmh to 1.26 for async profiler support~~ MINOR: Update jmh to 1.27 for async profiler support Dec 9, 2020

ijuma force-pushed the jmh-1.24 branch from 005aa52 to f735d9e Compare December 9, 2020 20:37

chia7712 approved these changes Dec 10, 2020

View reviewed changes

ijuma added 9 commits December 10, 2020 05:40

Exclude jopt from jmh project

35ec135

Improve jmh benchmarking docs

c00f2f5

JMH 1.25

f24e61a

WIP

a5ad3a9

JMH 1.27 and README improvements

bbdbc76

Fix compiler error with latest jmh and doc improvements

4993b7c

Readme fixes

97db755

Fix missing single quotes in readme

b64b1f1

ijuma added 2 commits December 10, 2020 05:40

Readme tweaks

67458a3

Adjust spotbugs exclusion to account for differences in the generated…

8e52198

… code

ijuma force-pushed the jmh-1.24 branch from f425972 to 8e52198 Compare December 10, 2020 13:41

ijuma commented Dec 10, 2020

View reviewed changes

chia7712 approved these changes Dec 10, 2020

View reviewed changes

lbradstreet reviewed Dec 10, 2020

View reviewed changes

Tweak "profiler good practice" text

0f5cd4d

ijuma merged commit 8cabd57 into apache:trunk Dec 11, 2020

ijuma deleted the jmh-1.24 branch December 11, 2020 01:56


		### Using JMH with async profiler

		It's good practice to check profiler output for microbenchmarks in order to verify that they are valid.


		java -jar <kafka-repo-dir>/jmh-benchmarks/build/libs/kafka-jmh-benchmarks-all.jar -f2 LRUCacheBenchmark

		### Writing benchmarks

Conversation

ijuma commented Aug 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

ijuma commented Aug 5, 2020

Uh oh!

ijuma commented Aug 11, 2020

Uh oh!

bbejeck left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lbradstreet commented Aug 22, 2020

Uh oh!

lbradstreet left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Using JMH GC profiler

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lbradstreet commented Oct 11, 2020

Uh oh!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

ijuma commented Dec 10, 2020

Uh oh!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ijuma commented Dec 10, 2020

Uh oh!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

ijuma commented Aug 5, 2020 •

edited

Loading

lbradstreet left a comment •

edited

Loading