ARROW-10031: [CI][Java] Support Java benchmark in Archery by kiszk · Pull Request #8210 · apache/arrow

kiszk · 2020-09-17T15:57:26Z

This PR supports Java benchmark in Ursabot. The implementation is based on this suggestion

Here are work items.

Support --language=[cpp|java] option in diff
Enable to build java binding
Enable to run Java benchmarks
Allows us to filter/select benchmarks
Enable to collect results
Apply the same changes to run and list

github-actions · 2020-09-17T16:05:45Z

https://issues.apache.org/jira/browse/ARROW-10031

liyafan82 · 2020-09-23T03:54:51Z

@kiszk Thank you for doing this.
Please note that when running the benchmarks, some flags should be configured properly.
They can be set through environmental variables:

ARROW_ENABLE_UNSAFE_MEMORY_ACCESS = true
ARROW_ENABLE_NULL_CHECK_FOR_GET = false

or through system properties:

arrow.enable_unsafe_memory_access = true
arrow.enable_null_check_for_get = false

kiszk · 2020-09-23T03:57:37Z

@liyafan82 Thank you for your comment. I will set these two properties as default for Java benchmarking
.

kiszk · 2020-11-11T05:40:30Z

At my end, I can generate the following JSON file by archery benchmark diff --language=java ...

@liyafan82 any comments regarding format and parameters are appreciated.

                                                  benchmark      baseline     contender  change %                                                                                                                                                                                                           counters
0      org.apache.arrow.vector.IntBenchmarks.setIntDirectly  22.500 us/op  11.209 us/op   -50.182  {'mode': 'avgt', 'threads': 1, 'warmups': 5, 'warmupTime': '10 s', 'measurements': 4, 'measurementTime': '10 s', 'jvmArgs': ['-Darrow.enable_null_check_for_get=false -Darrow.enable_unsafe_memory_access=true']}
2  org.apache.arrow.vector.IntBenchmarks.setWithValueHolder  19.031 us/op   6.627 us/op   -65.179  {'mode': 'avgt', 'threads': 1, 'warmups': 5, 'warmupTime': '10 s', 'measurements': 4, 'measurementTime': '10 s', 'jvmArgs': ['-Darrow.enable_null_check_for_get=false -Darrow.enable_unsafe_memory_access=true']}
1       org.apache.arrow.vector.IntBenchmarks.setWithWriter  32.626 us/op  10.246 us/op   -68.594  {'mode': 'avgt', 'threads': 1, 'warmups': 5, 'warmupTime': '10 s', 'measurements': 4, 'measurementTime': '10 s', 'jvmArgs': ['-Darrow.enable_null_check_for_get=false -Darrow.enable_unsafe_memory_access=true']}

liyafan82 · 2020-11-11T08:29:08Z

At my end, I can generate the following JSON file by archery benchmark diff --language=java ...

@liyafan82 any comments regarding format and parameters are appreciated.

                                                  benchmark      baseline     contender  change %                                                                                                                                                                                                           counters
0      org.apache.arrow.vector.IntBenchmarks.setIntDirectly  22.500 us/op  11.209 us/op   -50.182  {'mode': 'avgt', 'threads': 1, 'warmups': 5, 'warmupTime': '10 s', 'measurements': 4, 'measurementTime': '10 s', 'jvmArgs': ['-Darrow.enable_null_check_for_get=false -Darrow.enable_unsafe_memory_access=true']}
2  org.apache.arrow.vector.IntBenchmarks.setWithValueHolder  19.031 us/op   6.627 us/op   -65.179  {'mode': 'avgt', 'threads': 1, 'warmups': 5, 'warmupTime': '10 s', 'measurements': 4, 'measurementTime': '10 s', 'jvmArgs': ['-Darrow.enable_null_check_for_get=false -Darrow.enable_unsafe_memory_access=true']}
1       org.apache.arrow.vector.IntBenchmarks.setWithWriter  32.626 us/op  10.246 us/op   -68.594  {'mode': 'avgt', 'threads': 1, 'warmups': 5, 'warmupTime': '10 s', 'measurements': 4, 'measurementTime': '10 s', 'jvmArgs': ['-Darrow.enable_null_check_for_get=false -Darrow.enable_unsafe_memory_access=true']}

@kiszk Thanks for your effort. Generally, it looks great! Some minor comments:

It is clearer to rename title 'counters' to 'configuration'?
I am curious how are the benchmarks sorted, by 'change %'?

kiszk · 2020-11-11T08:48:01Z

For 1., I will rename the title for cpp and Java.

For 2, you are right as sorted here.

kiszk · 2020-11-13T06:08:57Z

@liyafan82 @fsaintjacques @kszucs Would it be possible to review this?

kiszk · 2020-11-13T06:10:32Z

The following commands should work:

archery benchmark list --langauge=java
archery benchmark run --langauge=java
archery benchmark diff --langauge=java

Here is an example

archery benchmark run  ARROW-10031  --output=out-java.json --language=java  --benchmark-filter=IntBench --java-options=-mx1g --build-extras=-Dfoo.bar=1 --benchmark-extras=-Dbar.foo=2

kiszk · 2020-11-13T06:16:37Z

java/performance/pom.xml

@liyafan82 Here is a change from #8245 . Since I cannot find to add -rf json or not to add it in <arguments>, the current implementation always generates jmh-result.json and change a file name if it will be overridden.

I would like to update here if there is a selection method to add -rf json or not to add it.

It looks good. Thanks.

kiszk · 2020-12-20T06:14:48Z

ping @liyafan82 @fsaintjacques @kszucs

liyafan82 · 2020-12-21T02:38:09Z

The Java changes look good to me. However, I am not farmiliar with the archery code. So @fsaintjacques @kszucs could you please take a look?

kszucs · 2020-12-21T11:08:16Z

Thanks for working on this! At the moment we can only try this out locally since we no longer maintain the buildbot/ursabot setup. I have limited time to review this, so I'd say that if the benchmarking using archery works locally then is should be good to go.

kiszk · 2021-01-11T09:07:46Z

I see. Thank you for sharing the latest status.

kiszk · 2021-01-11T09:09:10Z

@bkietz @xhochy May I ask to review this if possible?

xhochy · 2021-01-11T14:06:19Z

Leaving this for @bkietz , archery is not my area of expertise.

bkietz

In general, I'd recommend refactoring for less repetition of code between the classes for handling java and C++ builds

bkietz · 2021-01-11T15:18:12Z

dev/archery/archery/benchmark/compare.py

Instead, please convert to "items_per_second" or "bytes_per_second" in JavaMicrobenchmarkHarnessObservation.unit

bkietz · 2021-01-11T15:22:58Z

dev/archery/archery/benchmark/jmh.py

This comment doesn't apply to java benchmarks; seems copy pasted from GoogleBenchmarkCommand?

bkietz · 2021-01-11T15:24:00Z

dev/archery/archery/benchmark/jmh.py

Please put docstrings on the first line of function definitions

Also, please make it clear that you're using the strings "Benchmarks:" and "[INFO]" to delimit lines containing benchmark names

bkietz · 2021-01-11T15:26:31Z

dev/archery/archery/benchmark/jmh.py

Suggested change

benchmarks = False

bkietz · 2021-01-11T15:30:00Z

dev/archery/archery/benchmark/jmh.py

It's odd that this class inherits Maven instead of Command, especially since it has a member self.maven

Suggested change

class JavaMicrobenchmarkHarnessCommand(Maven):

class JavaMicrobenchmarkHarnessCommand(Command):

bkietz · 2021-01-11T15:43:41Z

dev/archery/archery/cli.py

This division seems unnecessary. Please just keep a single benchmark_list function (moving the branch on language into the with block)

bkietz · 2021-01-11T16:14:06Z

dev/archery/archery/lang/java.py

I'd recommend doing the cast-to-list here; if an iterator were passed for build_extras then it'd be depleted after the first access to build_definitions:

Suggested change

self.build_extras = build_extras

self.benchmark_extras = benchmark_extras

@property

def build_definitions(self):

extras = list(self.build_extras) if self.build_extras else []

return extras

self.build_extras = list(build_extras) if build_extras else []

self.benchmark_extras = list(benchmark_extras) if benchmark_extras else []

@property

def build_definitions(self):

return self.build_extras

I see that CppConfiguration has this same flaw; it's not necessary for you to address it there too

bkietz · 2021-01-11T16:15:43Z

dev/archery/archery/cli.py

This division also seems unnecessary; CppBenchmarkRunner and JavaBenchmarkRunner seem similar enough to share more code here

bkietz · 2021-01-11T16:18:12Z

dev/archery/archery/cli.py

Again, why separate the languages' benchmarks? It seems especially valuable to have a clearly unified strategy for comparison of benchmarks

This separation comes from @click.pass_context. To use different options between two languages (e.g. java-home or cxx-flags), my understanding was that we need different methods with the @click.pass_context.

Let me try to have one method with @click.pass_context by lazily parse arguments for each language.

bkietz · 2021-01-11T16:22:18Z

dev/archery/archery/benchmark/runner.py

Suggested change

directory. If so, it creates a JavaenchmarkRunner with this existing

directory. If so, it creates a JavaBenchmarkRunner with this existing

Again, seems like this code didn't need to be repeated

emkornfield · 2021-01-31T21:38:05Z

@kiszk @bkietz are there more revisions needed here? (it looks like this at least needs a rebase?)

kiszk · 2021-02-01T04:58:30Z

@emkornfield @bkietz Thank you for kind ping. Yes, I need an update since I was swamped last month. I will update this PR this week.

emkornfield · 2021-02-28T21:26:59Z

CC @dianaclarke who I think has also been working on continuous benchmarking.

kiszk · 2021-04-12T14:23:33Z

Here is an example.

% archery benchmark diff --language=java --benchmark-filter="setWith"  HEAD HEAD~1
...
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Non-regressions: (2)
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
                                                benchmark            baseline           contender  change %                                                                                                                                                                                                     configurations
 org.apache.arrow.vector.IntBenchmarks.setWithValueHolder  165.925K items/sec  165.898K items/sec    -0.016  {'mode': 'avgt', 'threads': 1, 'warmups': 5, 'warmupTime': '10 s', 'measurements': 1, 'measurementTime': '10 s', 'jvmArgs': ['-Darrow.enable_null_check_for_get=false -Darrow.enable_unsafe_memory_access=true']}
      org.apache.arrow.vector.IntBenchmarks.setWithWriter  222.780K items/sec  221.768K items/sec    -0.454  {'mode': 'avgt', 'threads': 1, 'warmups': 5, 'warmupTime': '10 s', 'measurements': 1, 'measurementTime': '10 s', 'jvmArgs': ['-Darrow.enable_null_check_for_get=false -Darrow.enable_unsafe_memory_access=true']}

kiszk · 2021-04-12T14:25:25Z

@emkornfield @bkietz @dianaclarke @liyafan82 I am very sorry for being late to address review comments.

Now, I addressed all of the comments and rebased with the master. Would it be possible to review this PR again?

kszucs · 2021-04-14T12:39:15Z

@ursabot --help

ursabot · 2021-04-14T12:39:16Z

Supported benchmark command examples:

@ursabot benchmark help

To run all benchmarks:
@ursabot please benchmark

To filter benchmarks by language:
@ursabot please benchmark lang=Python
@ursabot please benchmark lang=C++
@ursabot please benchmark lang=R

To filter Python and R benchmarks by name:
@ursabot please benchmark name=file-write
@ursabot please benchmark name=file-write lang=Python
@ursabot please benchmark name=file-.*

To filter C++ benchmarks by archery --suite-filter and --benchmark-filter:
@ursabot please benchmark command=cpp-micro --suite-filter=arrow-compute-vector-selection-benchmark --benchmark-filter=TakeStringRandomIndicesWithNulls/262144/2 --iterations=3

For other command=cpp-micro options, please see https://github.com/ursacomputing/benchmarks/blob/main/benchmarks/cpp_micro_benchmarks.py

kszucs · 2021-04-14T12:40:36Z

@ursabot please benchmark lang=C++

ursabot · 2021-04-14T12:40:40Z

Benchmark runs are scheduled for baseline = 5b08205 and contender = 5d70561. Results will be available as each benchmark for each run completes:
[Finished] ursa-i9-9960x: https://conbench.ursa.dev/compare/runs/b04151dd-74b7-4cb5-ac7b-35af58124af7...c28547fd-3722-430f-952e-1b08e321bcaf/
[Finished] ursa-thinkcentre-m75q: https://conbench.ursa.dev/compare/runs/0af6683d-a459-4750-868d-e2bbdc5c49dc...761cb228-a197-446a-bfe9-a2834c44ba06/
[Finished] ec2-t3-large-us-east-2: https://conbench.ursa.dev/compare/runs/1635513d-25fa-4ec8-9937-b248f6c49ef4...c561174b-c02d-44e9-a6dc-b2a0bd010474/
[Finished] ec2-t3-xlarge-us-east-2: https://conbench.ursa.dev/compare/runs/ccf5c2c4-e672-4c37-a511-01a721a8e276...3b4ae039-2fe4-419b-b47a-4dc9a4d0e400/

kszucs · 2021-04-14T12:52:26Z

I wouldn't be too pedantic here, we can address any issues later. Nice addition, thanks @kiszk!

I submitted a conbench build to check that the archery benchmark command remained compatible.

emkornfield · 2021-04-25T03:51:32Z

Should this be merged?

ursabot · 2021-04-25T03:52:20Z

Benchmark runs are scheduled for baseline = 5b08205 and contender = 5d70561. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-large-us-east-2
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️2.08% ⬆️2.14%] ursa-thinkcentre-m75q

kiszk · 2021-04-29T03:29:24Z

@bkietz @dianaclarke @liyafan82 Would you have additional comments?

liyafan82 · 2021-04-29T06:27:39Z

@bkietz @dianaclarke @liyafan82 Would you have additional comments?

I have no more comments. Hope this PR will be merged soon.

kou

+1

If wee need more refactoring, we can work on it as a follow-up task.

kiszk · 2021-05-01T02:26:05Z

Thanks very much for your comments

This PR supports Java benchmark in Ursabot. The implementation is based on [this suggestion](https://mail-archives.apache.org/mod_mbox/arrow-dev/202008.mbox/%3cCABNn7+q35j7QWsHJBX8omdewKT+F1p_M7r1_F6szs4dqc+Luyg@mail.gmail.com%3e) Here are work items. - [x] Support `--language=[cpp|java]` option in `diff` - [x] Enable to build java binding - [x] Enable to run Java benchmarks - [x] Allows us to filter/select benchmarks - [x] Enable to collect results - [x] Apply the same changes to `run` and `list` Closes apache#8210 from kiszk/ARROW-10031 Authored-by: Kazuaki Ishizaki <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>

kiszk force-pushed the ARROW-10031 branch 3 times, most recently from d719b0d to 0e47ab6 Compare September 22, 2020 00:36

kiszk mentioned this pull request Nov 5, 2020

ARROW-9861: [Java] Support big-endian in DecimalVector #8056

Closed

kiszk force-pushed the ARROW-10031 branch 3 times, most recently from 3ebd9f6 to 8cdd1ea Compare November 13, 2020 01:32

kiszk marked this pull request as ready for review November 13, 2020 04:59

kiszk commented Nov 13, 2020

View reviewed changes

kszucs changed the title ~~ARROW-10031: [CI][Java] Support Java benchmark in Ursabot~~ ARROW-10031: [CI][Java] Support Java benchmark in Archery Dec 21, 2020

bkietz self-requested a review January 11, 2021 15:58

bkietz requested changes Jan 11, 2021

View reviewed changes

jorgecarleitao force-pushed the master branch from d4608a9 to 356c300 Compare February 14, 2021 12:09

kiszk added 7 commits April 10, 2021 17:00

fix lint errors

da46fca

fix lint errors

c17f50c

address trivial comments

e924ba2

address trivial comments

6e1161b

normalize unit

513a7ca

refactor to reduce more splits

1522558

updates

150d19c

kiszk force-pushed the ARROW-10031 branch from 68ea469 to 150d19c Compare April 12, 2021 10:02

github-actions bot added the Component: Java label Apr 12, 2021

fix lint errors

5d70561

kszucs approved these changes Apr 14, 2021

View reviewed changes

kou approved these changes Apr 30, 2021

View reviewed changes

kou closed this in 2ece340 Apr 30, 2021

asfimport mentioned this pull request May 1, 2021

[Java] Support Java benchmark in Archery #26053

Closed

	class JavaMicrobenchmarkHarnessCommand(Maven):
	class JavaMicrobenchmarkHarnessCommand(Command):

-        self.build_extras = build_extras
-        self.benchmark_extras = benchmark_extras
-    @property
-    def build_definitions(self):
-        extras = list(self.build_extras) if self.build_extras else []
-        return extras
+        self.build_extras = list(build_extras) if build_extras else []
+        self.benchmark_extras = list(benchmark_extras) if benchmark_extras else []
+    @property
+    def build_definitions(self):
+        return self.build_extras

	directory. If so, it creates a JavaenchmarkRunner with this existing
	directory. If so, it creates a JavaBenchmarkRunner with this existing

Conversation

kiszk commented Sep 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Sep 17, 2020

Uh oh!

liyafan82 commented Sep 23, 2020

Uh oh!

kiszk commented Sep 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kiszk commented Nov 11, 2020

Uh oh!

liyafan82 commented Nov 11, 2020

Uh oh!

kiszk commented Nov 11, 2020

Uh oh!

kiszk commented Nov 13, 2020

Uh oh!

kiszk commented Nov 13, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kiszk commented Dec 20, 2020

Uh oh!

liyafan82 commented Dec 21, 2020

Uh oh!

kszucs commented Dec 21, 2020

Uh oh!

kiszk commented Jan 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kiszk commented Jan 11, 2021

Uh oh!

xhochy commented Jan 11, 2021

Uh oh!

bkietz 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!

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!

emkornfield commented Jan 31, 2021

Uh oh!

kiszk commented Feb 1, 2021

Uh oh!

emkornfield commented Feb 28, 2021

Uh oh!

kiszk commented Apr 12, 2021

Uh oh!

kiszk commented Apr 12, 2021

Uh oh!

kszucs commented Apr 14, 2021

Uh oh!

ursabot commented Apr 14, 2021

Uh oh!

kszucs commented Apr 14, 2021

kiszk commented Sep 17, 2020 •

edited

Loading

kiszk commented Sep 23, 2020 •

edited

Loading

kiszk commented Jan 11, 2021 •

edited

Loading

ursabot commented Apr 14, 2021 •

edited

Loading

emkornfield commented Apr 25, 2021 •

edited

Loading

ursabot commented Apr 25, 2021 •

edited

Loading