ARROW-10031: [CI][Java] Support Java benchmark in Archery

[kiszk]

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]

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

[liyafan82]

@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]

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

[kiszk]

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]

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:

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

[kiszk]

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

For 2, you are right as sorted here.

[kiszk]

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

[kiszk]

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]

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

[liyafan82]

It looks good. Thanks.

[kiszk]

ping @liyafan82@fsaintjacques@kszucs

[liyafan82]

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]

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]

I see. Thank you for sharing the latest status.

[kiszk]

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

[xhochy]

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]

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

[bkietz]

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

[bkietz]

Please put docstrings on the first line of function definitions

[bkietz]

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

[bkietz]

Suggested change

benchmarks = False

[bkietz]

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

Suggested change

	classJavaMicrobenchmarkHarnessCommand(Maven):
	classJavaMicrobenchmarkHarnessCommand(Command):

[bkietz]

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

[bkietz]

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
	defbuild_definitions(self):
	extras=list(self.build_extras) ifself.build_extraselse []
	returnextras
	self.build_extras=list(build_extras) ifbuild_extraselse []
	self.benchmark_extras=list(benchmark_extras) ifbenchmark_extraselse []
	@property
	defbuild_definitions(self):
	returnself.build_extras

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

[bkietz]

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

[bkietz]

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

[kiszk]

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]

Suggested change

	directory. Ifso, itcreatesaJavaenchmarkRunnerwiththisexisting
	directory. Ifso, itcreatesaJavaBenchmarkRunnerwiththisexisting

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

[emkornfield]

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

[kiszk]

@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]

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

[kiszk]

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]

@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]

@ursabot --help

[ursabot]

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]

@ursabot please benchmark lang=C++

[ursabot]

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]

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]

Should this be merged?

[ursabot]

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]

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

[liyafan82]

@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]

Thanks very much for your comments

[kiszk]

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]

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

[liyafan82]

@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]

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

[kiszk]

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]

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:

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

[kiszk]

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

For 2, you are right as sorted here.

[kiszk]

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

[kiszk]

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]

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

[liyafan82]

It looks good. Thanks.

[kiszk]

ping @liyafan82@fsaintjacques@kszucs

[liyafan82]

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]

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]

I see. Thank you for sharing the latest status.

[kiszk]

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

[xhochy]

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]

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

[bkietz]

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

[bkietz]

Please put docstrings on the first line of function definitions

[bkietz]

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

[bkietz]

Suggested change

benchmarks = False

[bkietz]

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

Suggested change

	classJavaMicrobenchmarkHarnessCommand(Maven):
	classJavaMicrobenchmarkHarnessCommand(Command):

[bkietz]

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

[bkietz]

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
	defbuild_definitions(self):
	extras=list(self.build_extras) ifself.build_extraselse []
	returnextras
	self.build_extras=list(build_extras) ifbuild_extraselse []
	self.benchmark_extras=list(benchmark_extras) ifbenchmark_extraselse []
	@property
	defbuild_definitions(self):
	returnself.build_extras

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

[bkietz]

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

[bkietz]

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

[kiszk]

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]

Suggested change

	directory. Ifso, itcreatesaJavaenchmarkRunnerwiththisexisting
	directory. Ifso, itcreatesaJavaBenchmarkRunnerwiththisexisting

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

[emkornfield]

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

[kiszk]

@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]

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

[kiszk]

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]

@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]

@ursabot --help

[ursabot]

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]

@ursabot please benchmark lang=C++

[ursabot]

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]

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]

Should this be merged?

[ursabot]

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]

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

[liyafan82]

@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]

Thanks very much for your comments

[kiszk]

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]

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

[liyafan82]

@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]

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

[kiszk]

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]

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:

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

[kiszk]

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

For 2, you are right as sorted here.

[kiszk]

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

[kiszk]

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]

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

[liyafan82]

It looks good. Thanks.

[kiszk]

ping @liyafan82@fsaintjacques@kszucs

[liyafan82]

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]

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]

I see. Thank you for sharing the latest status.

[kiszk]

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

[xhochy]

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]

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

[bkietz]

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

[bkietz]

Please put docstrings on the first line of function definitions

[bkietz]

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

[bkietz]

Suggested change

benchmarks = False

[bkietz]

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

Suggested change

	classJavaMicrobenchmarkHarnessCommand(Maven):
	classJavaMicrobenchmarkHarnessCommand(Command):

[bkietz]

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

[bkietz]

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
	defbuild_definitions(self):
	extras=list(self.build_extras) ifself.build_extraselse []
	returnextras
	self.build_extras=list(build_extras) ifbuild_extraselse []
	self.benchmark_extras=list(benchmark_extras) ifbenchmark_extraselse []
	@property
	defbuild_definitions(self):
	returnself.build_extras

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

[bkietz]

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

[bkietz]

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

[kiszk]

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]

Suggested change

	directory. Ifso, itcreatesaJavaenchmarkRunnerwiththisexisting
	directory. Ifso, itcreatesaJavaBenchmarkRunnerwiththisexisting

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

[emkornfield]

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

[kiszk]

@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]

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

[kiszk]

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]

@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]

@ursabot --help

[ursabot]

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]

@ursabot please benchmark lang=C++

[ursabot]

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]

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]

Should this be merged?

[ursabot]

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]

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

[liyafan82]

@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]

Thanks very much for your comments

[kiszk]

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]

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

[liyafan82]

@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]

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

[kiszk]

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]

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:

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

[kiszk]

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

For 2, you are right as sorted here.

[kiszk]

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

[kiszk]

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]

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

[liyafan82]

It looks good. Thanks.

[kiszk]

ping @liyafan82@fsaintjacques@kszucs

[liyafan82]

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]

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]

I see. Thank you for sharing the latest status.

[kiszk]

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

[xhochy]

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]

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

[bkietz]

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

[bkietz]

Please put docstrings on the first line of function definitions

[bkietz]

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

[bkietz]

Suggested change

benchmarks = False

[bkietz]

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

Suggested change

	classJavaMicrobenchmarkHarnessCommand(Maven):
	classJavaMicrobenchmarkHarnessCommand(Command):

[bkietz]

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

[bkietz]

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
	defbuild_definitions(self):
	extras=list(self.build_extras) ifself.build_extraselse []
	returnextras
	self.build_extras=list(build_extras) ifbuild_extraselse []
	self.benchmark_extras=list(benchmark_extras) ifbenchmark_extraselse []
	@property
	defbuild_definitions(self):
	returnself.build_extras

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

[bkietz]

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

[bkietz]

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

[kiszk]

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]

Suggested change

	directory. Ifso, itcreatesaJavaenchmarkRunnerwiththisexisting
	directory. Ifso, itcreatesaJavaBenchmarkRunnerwiththisexisting

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

[emkornfield]

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

[kiszk]

@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]

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

[kiszk]

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]

@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]

@ursabot --help

[ursabot]

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]

@ursabot please benchmark lang=C++

[ursabot]

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]

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]

Should this be merged?

[ursabot]

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]

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

[liyafan82]

@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]

Thanks very much for your comments

[kiszk]

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]

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

[liyafan82]

@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]

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

[kiszk]

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]

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:

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

[kiszk]

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

For 2, you are right as sorted here.

[kiszk]

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

[kiszk]

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]

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

[liyafan82]

It looks good. Thanks.

[kiszk]

ping @liyafan82@fsaintjacques@kszucs

[liyafan82]

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]

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]

I see. Thank you for sharing the latest status.

[kiszk]

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

[xhochy]

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]

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

[bkietz]

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

[bkietz]

Please put docstrings on the first line of function definitions

[bkietz]

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

[bkietz]

Suggested change

benchmarks = False

[bkietz]

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

Suggested change

	classJavaMicrobenchmarkHarnessCommand(Maven):
	classJavaMicrobenchmarkHarnessCommand(Command):

[bkietz]

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

[bkietz]

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
	defbuild_definitions(self):
	extras=list(self.build_extras) ifself.build_extraselse []
	returnextras
	self.build_extras=list(build_extras) ifbuild_extraselse []
	self.benchmark_extras=list(benchmark_extras) ifbenchmark_extraselse []
	@property
	defbuild_definitions(self):
	returnself.build_extras

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

[bkietz]

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

[bkietz]

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

[kiszk]

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]

Suggested change

	directory. Ifso, itcreatesaJavaenchmarkRunnerwiththisexisting
	directory. Ifso, itcreatesaJavaBenchmarkRunnerwiththisexisting

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

[emkornfield]

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

[kiszk]

@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]

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

[kiszk]

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]

@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]

@ursabot --help

[ursabot]

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]

@ursabot please benchmark lang=C++

[ursabot]

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]

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]

Should this be merged?

[ursabot]

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]

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

[liyafan82]

@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]

Thanks very much for your comments

[kiszk]

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]

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

[liyafan82]

@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]

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

[kiszk]

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]

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:

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

[kiszk]

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

For 2, you are right as sorted here.

[kiszk]

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

[kiszk]

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]

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

[liyafan82]

It looks good. Thanks.

[kiszk]

ping @liyafan82@fsaintjacques@kszucs

[liyafan82]

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]

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]

I see. Thank you for sharing the latest status.

[kiszk]

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

[xhochy]

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]

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

[bkietz]

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

[bkietz]

Please put docstrings on the first line of function definitions

[bkietz]

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

[bkietz]

Suggested change

benchmarks = False

[bkietz]

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

Suggested change

	classJavaMicrobenchmarkHarnessCommand(Maven):
	classJavaMicrobenchmarkHarnessCommand(Command):

[bkietz]

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

[bkietz]

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
	defbuild_definitions(self):
	extras=list(self.build_extras) ifself.build_extraselse []
	returnextras
	self.build_extras=list(build_extras) ifbuild_extraselse []
	self.benchmark_extras=list(benchmark_extras) ifbenchmark_extraselse []
	@property
	defbuild_definitions(self):
	returnself.build_extras

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

[bkietz]

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

[bkietz]

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

[kiszk]

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]

Suggested change

	directory. Ifso, itcreatesaJavaenchmarkRunnerwiththisexisting
	directory. Ifso, itcreatesaJavaBenchmarkRunnerwiththisexisting

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

[emkornfield]

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

[kiszk]

@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]

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

[kiszk]

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]

@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]

@ursabot --help

[ursabot]

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]

@ursabot please benchmark lang=C++

[ursabot]

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]

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]

Should this be merged?

[ursabot]

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]

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

[liyafan82]

@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]

Thanks very much for your comments

[kiszk]

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]

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

[liyafan82]

@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]

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

[kiszk]

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]

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:

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

[kiszk]

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

For 2, you are right as sorted here.

[kiszk]

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

[kiszk]

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]

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

[liyafan82]

It looks good. Thanks.

[kiszk]

ping @liyafan82@fsaintjacques@kszucs

[liyafan82]

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]

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]

I see. Thank you for sharing the latest status.

[kiszk]

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

[xhochy]

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]

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

[bkietz]

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

[bkietz]

Please put docstrings on the first line of function definitions

[bkietz]

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

[bkietz]

Suggested change

benchmarks = False

[bkietz]

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

Suggested change

	classJavaMicrobenchmarkHarnessCommand(Maven):
	classJavaMicrobenchmarkHarnessCommand(Command):

[bkietz]

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

[bkietz]

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
	defbuild_definitions(self):
	extras=list(self.build_extras) ifself.build_extraselse []
	returnextras
	self.build_extras=list(build_extras) ifbuild_extraselse []
	self.benchmark_extras=list(benchmark_extras) ifbenchmark_extraselse []
	@property
	defbuild_definitions(self):
	returnself.build_extras

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

[bkietz]

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

[bkietz]

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

[kiszk]

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]

Suggested change

	directory. Ifso, itcreatesaJavaenchmarkRunnerwiththisexisting
	directory. Ifso, itcreatesaJavaBenchmarkRunnerwiththisexisting

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

[emkornfield]

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

[kiszk]

@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]

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

[kiszk]

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]

@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]

@ursabot --help

[ursabot]

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]

@ursabot please benchmark lang=C++

[ursabot]

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]

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]

Should this be merged?

[ursabot]

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]

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

[liyafan82]

@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]

Thanks very much for your comments

[kiszk]

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]

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

[liyafan82]

@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]

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

[kiszk]

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]

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:

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

[kiszk]

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

For 2, you are right as sorted here.

[kiszk]

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

[kiszk]

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]

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

[liyafan82]

It looks good. Thanks.

[kiszk]

ping @liyafan82@fsaintjacques@kszucs

[liyafan82]

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]

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]

I see. Thank you for sharing the latest status.

[kiszk]

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

[xhochy]

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]

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

[bkietz]

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

[bkietz]

Please put docstrings on the first line of function definitions

[bkietz]

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

[bkietz]

Suggested change

benchmarks = False

[bkietz]

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

Suggested change

	classJavaMicrobenchmarkHarnessCommand(Maven):
	classJavaMicrobenchmarkHarnessCommand(Command):

[bkietz]

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

[bkietz]

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
	defbuild_definitions(self):
	extras=list(self.build_extras) ifself.build_extraselse []
	returnextras
	self.build_extras=list(build_extras) ifbuild_extraselse []
	self.benchmark_extras=list(benchmark_extras) ifbenchmark_extraselse []
	@property
	defbuild_definitions(self):
	returnself.build_extras

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

[bkietz]

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

[bkietz]

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

[kiszk]

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]

Suggested change

	directory. Ifso, itcreatesaJavaenchmarkRunnerwiththisexisting
	directory. Ifso, itcreatesaJavaBenchmarkRunnerwiththisexisting

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

[emkornfield]

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

[kiszk]

@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]

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

[kiszk]

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]

@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]

@ursabot --help

[ursabot]

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]

@ursabot please benchmark lang=C++

[ursabot]

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]

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]

Should this be merged?

[ursabot]

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]

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

[liyafan82]

@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]

Thanks very much for your comments