-
Notifications
You must be signed in to change notification settings - Fork 323
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ability to invoke all std benchmarks via jmh #7519
Ability to invoke all std benchmarks via jmh #7519
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect and sufficient, in my opinion.
lib/scala/bench-processor/src/main/java/org/enso/benchmarks/processor/BenchProcessor.java
Show resolved
Hide resolved
lib/scala/bench-processor/src/main/java/org/enso/benchmarks/processor/BenchProcessor.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks perfect! ❤️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can say we don't need teardown
right now. As such, please let's keep this pull request simple and focused. Let's use existing Enso language lazy atom fields concept to get proper setup
behavior without changing the BenchProcessor
, without introducing set_setup
and set_teardown
and without having problems to invoke these methods from JMH.
Sounds like a good idea. I reverted all the commits that introduced the special API for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The direction looks good and I agree with @JaroslavTulach on setup/teardown - let's keep it simple. I think computing the lazy values on first warmup iteration should be acceptable - it's warmup after all. We just should make sure that they are indeed not recomputed in the measurement phase.
And let's ensure the data for benchmarks is always computed lazily so it is not computed in the 'gathering benchmark structure' phase, as it may slow it down.
…std-benchmarks-via-JMH # Conflicts: # test/Benchmarks/src/Main.enso # test/Benchmarks/src/Numeric.enso
This reverts commit bdd4544
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the CI is green. I think the current state is good. Approving.
builder = Vector.new_builder | ||
|
||
# Vector | ||
builder.append Array_Proxy_Bench.collect_benches |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't all the collect_benches
method accept a Bench.build
er? Then this would be:
Bench.build builder ->
Array_Proxy_Bench.collect_benches builder
Distinct.callect_benches builder
...
Then we wouldn't need to iterate Vector
, but rather:
main =
all_benchmarks . run_main
…std-benchmarks-via-JMH # Conflicts: # test/Benchmarks/src/Column_Numeric.enso
Closes #7489
Pull Request Description
Important Notes
setup
orteardown
methods.Text/compare
is in 3ca6cd7Text/compare
takes 7 seconds, with lazy setup, the collection takes 160 ms.Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
./run ide build
.