Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

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

Jump to bottom

JMH-based benchmark framework for contributors #5061

Merged
merged 8 commits into from May 25, 2016
Merged

JMH-based benchmark framework for contributors #5061

merged 8 commits into from May 25, 2016

Conversation

performantdata
Copy link
Contributor

Turns test/benchmarks into an SBT project that uses SBT-JMH.

This is in follow-up to my plans mentioned on scala-internals.

@scala-jenkins scala-jenkins added this to the 2.11.9 milestone Mar 23, 2016
@SethTisue
Copy link
Member

presumably the integrate-ide failure is scala/scala-dev#104 rather than anything specific to this PR

@retronym
Copy link
Member

Great stuff. I had toyed with this idea in https://github.com/retronym/scala-jmh-suite, which includes some plumbing to enable collection of Java Flight Recorder profiles of benchmarks.

@performantdata
Copy link
Contributor Author

Thanks. Thanks for the URL. I'll need to track memory usage soon. I also want to build my Gnuplot graphs automatically. More to come after this PR.

@performantdata
Copy link
Contributor Author

@retronym, I'm going with JOL, since it gives me easy access to memory usage from the teardown code. Can't figure how I'd do that with Flight Recorder.

@retronym
Copy link
Member

The more tools in the box, the better. I'm mostly after a convienient way to turn on Flight recorder when analysing benchmarks manually.

@performantdata performantdata mentioned this pull request Mar 29, 2016
Closed
@SethTisue SethTisue mentioned this pull request Mar 30, 2016
Closed
@performantdata performantdata mentioned this pull request Apr 2, 2016
Closed
@performantdata performantdata mentioned this pull request Apr 19, 2016
Merged
@adriaanm
Copy link
Contributor

/nothingtoseehere -- The failure is due to scala/scala-dev#104, not this PR.

@performantdata
Copy link
Contributor Author

review by @Ichoran or @axel22 ?

This was referenced Apr 25, 2016
Closed
Merged
Ichoran
Ichoran reviewed Apr 30, 2016
View reviewed changes
test/benchmarks/README.md

## running a benchmark

The benchmarks require first building Scala into `../../build/pack`, using Ant.
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it matter whether it's Ant or SBT?

@adriaanm
Copy link
Contributor

adriaanm commented May 3, 2016

I've added the suffix to tell the CI to only build the last commit, to help in load balancing. Let's do a rebuild when the other commits are done.

@adriaanm adriaanm changed the title JMH-based benchmark framework [ci: last-only] JMH-based benchmark framework May 4, 2016
@adriaanm
Copy link
Contributor

adriaanm commented May 4, 2016 •
edited

/rebuild

@adriaanm
Copy link
Contributor

adriaanm commented May 4, 2016

I removed the suffix that tells the CI to only build the last commit and then /rebuild. Hopefully all-green in a couple hours. ⏳

@Ichoran
Copy link
Contributor

Ichoran commented May 14, 2016

@performantdata - I'm snowed under with commitments but I'll try to get to it sometime this weekend (probably Sunday afternoon).

Ichoran
Ichoran reviewed May 16, 2016
View reviewed changes
test/benchmarks/src/main/scala/scala/collection/mutable/OpenHashMapRunner.scala
import java.io.OutputStreamWriter
import java.io.PrintWriter
import scala.collection.JavaConversions
import scala.language.existentials
Copy link
Contributor

Choose a reason for hiding this comment

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

This import is kind of lost here, which is contrary to the point of requiring the import in the first place. Can you move it to the top?

@Ichoran
Copy link
Contributor

Ichoran commented May 16, 2016

Getting close! I think there's just one real bug--or maybe it's just unclear, in which case you should leave a comment explaining why the right method is being benchmarked. The rest is just random style stuff. The last step will be to squash the commits into one; there doesn't seem to be any particular value to leaving them separate is there? (You might want to leave a branch with them separate in your repository, but it seems unnecessary for the main one.)

@performantdata
Copy link
Contributor Author

So, it's not a bug. I can address the style stuff in a subsequent commit in the PRs that I'll soon be benchmarking but haven't opened yet. Otherwise...when would you finish review?

@Ichoran
Copy link
Contributor

Ichoran commented May 21, 2016

Anyway, the tests look technically correct. There doesn't seem to be any reason to keep the separate commits, so you should squash them for the PR. The style could use a little improvement, and I'm a little wary of a promissory note instead of an actual fix (at least for most of them--the single get_Int call is probably fine, though again, I'm not sure the reason to not change it is very compelling). But if someone wants to merge this on technical soundness, it's ready.

@Ichoran
Copy link
Contributor

Ichoran commented May 21, 2016

And thank you for writing a JMH harness for testing library performance! It's long overdue in my opinion.

@Ichoran
Copy link
Contributor

Ichoran commented May 22, 2016

It almost does, but not completely, which is why I avoided the normal phrase. It would look good if the last few issues were fixed; as it is it's technically sound so could be merged anyway.

@retronym
Copy link
Member

I'm going to merge this one. Thanks for this effort, @performantdata. I think the commit history is clean enough to merge without squashing in this instance.

@retronym retronym merged commit cba585d into scala:2.11.x May 25, 2016
@performantdata
Copy link
Contributor Author

Uh...OK, thanks. I'm currently working on the fixes that Rex asked for, and adding AnyRef key tests. Another PR, I guess.

Thanks, Rex!

@performantdata performantdata deleted the benchmark-framework branch May 25, 2016 03:28
@performantdata performantdata mentioned this pull request May 28, 2016
Merged
@Ichoran Ichoran mentioned this pull request May 31, 2016
Closed
@SethTisue SethTisue added the release-notes worth highlighting in next release notes label Feb 22, 2017
@SethTisue
Copy link
Member

we don't normally release-note contributor-facing things, but perhaps we really want to draw attention to this? not sure, untag if you disagree

@SethTisue SethTisue changed the title JMH-based benchmark framework JMH-based benchmark framework for contributors Apr 14, 2017
@SethTisue SethTisue modified the milestones: 2.11.11, 2.11.9 May 1, 2017
@SethTisue
Copy link
Member

I also want to build my Gnuplot graphs automatically. More to come after this PR.

@performantdata @szeiger well I know quite some time has passed, but I'm wondering if you, or anyone else watching the repo, has anything that does this. even if it's not fully automatic, any sort of instructions I could follow. I'd love to be able to generate nice graphs like Stefan did over at #8534

@SethTisue
Copy link
Member

SethTisue commented Aug 18, 2021 •
edited

ah, @retronym has directed me towards https://github.com/lightbend/benchdb

I'll take it for a spin. (and perhaps if I'm feeling especially virtuous, I will add something about it to the benchmarking readme...)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
release-notes worth highlighting in next release notes
Projects
None yet
Milestone
2.11.11
6 participants
@performantdata @SethTisue @retronym @adriaanm @Ichoran @scala-jenkins