Skip to content

Conversation

@rafaelfranca
Copy link
Collaborator

@rafaelfranca rafaelfranca commented Nov 21, 2025

I had to spent some time understanding how the harness worked in this project. I realized that there were multiple directories for them and didn't know why.

I later realized that some were different harness implementation, and the harness folder was both the implementation and the framework.

I made this change the separate those two concepts, and remove the need to have different directories for implementation.

The framework lives in lib/harness and the implementations live in harness, in separated files (ractor.rb, perf.rb, etc.)

@rafaelfranca rafaelfranca requested a review from a team November 21, 2025 22:44
@k0kubun
Copy link
Member

k0kubun commented Nov 21, 2025

https://github.com/ruby/ruby-bench/actions/runs/19585546525/job/56093508019?pr=446#step:4:873
Running benchmark "railsbench" (46/67)

/home/runner/work/ruby-bench/ruby-bench/lib/harness/extra.rb:62: warning: already initialized constant MIN_BENCH_TIME
/home/runner/work/ruby-bench/ruby-bench/harness/default.rb:10: warning: previous definition of MIN_BENCH_TIME was here

Could you fix this warning? (I'm sorry if it wasn't new)

/home/runner/work/ruby-bench/ruby-bench/harness/stackprof.rb:72:in 'Object#run_benchmark': uninitialized constant StackProf (NameError)

StackProf.run(out: out, **opts) do
^^^^^^^^^

Do you know why the stackprof harness is loaded on the benchmark-default CI?

@rafaelfranca rafaelfranca force-pushed the rmf-better-harness branch 3 times, most recently from eccdfb4 to d3153ab Compare November 24, 2025 20:46
README.md Outdated

```
ruby benchmarks/some_benchmark.rb
./run_once.rb benchmarks/some_benchmark.rb
Copy link
Member

Choose a reason for hiding this comment

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

Being able to run a benchmark with just ruby benchmark.rb is a fundamental interface to this project. The low distance between ruby and benchmark code makes it easy to run and tweak for JIT work. I'm against sacrificing this for code organization reasons.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also it makes e.g. JIT bisect easier

Copy link
Member

Choose a reason for hiding this comment

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

(aside: I was actually looking for this section yesterday but failed to find it, probably I searched for -Iharness or so. I recall this section because I wrote it)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can't you still run ruby benchmarks/some_benchmark? You just need to use -r with an path:

ruby -r./harness/perf benchmarks/some_benchmark.rb

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

BTW, this isn't only about code organization. The interface to select harness isn't good either. Playing with load paths to be able to select harness isn't usual and it is confusing. I also realize that introducing a script doesn't work, although it doesn't prevent you to use ruby to run the benchmarks. I think of the options, the least worse is using environment variables, in addition to the -r. I'll change it to do that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I put back to the documentation ruby benchmarks/some_benchmark.rb as the preferred way to run single benchmarks. I also added support to set the harness using environment variable.

@rafaelfranca rafaelfranca force-pushed the rmf-better-harness branch 3 times, most recently from c82ec54 to 82e0b03 Compare November 27, 2025 19:50
@rafaelfranca rafaelfranca requested a review from XrXr November 27, 2025 19:51
Use file based harness instead of directory based harness
… selection

So we don't need to deal with the `-r` ruby argument directly for
selecting harnesses.
This allow `ruby` script direct calls to still select the harness without
using any wrapper script.
@eregon
Copy link
Member

eregon commented Dec 2, 2025

Given there is a once harness, maybe we should just remove run_once.sh and run_once.rb?
When running a single iteration of a benchmark (the meaning of once in this context) I don't think the harness matters much, with the one exception maybe of Ractors (if that's useful then maybe we can keep the trivial run_once.sh script).

My concern is I think we should not introduce "yet another way to run benchmarks". To be fair run_once.rb is pretty small but it seems unnecessary to me, I think HARNESS=once ruby some/benchmark.rb is enough.

Copy link
Member

@eregon eregon left a comment

Choose a reason for hiding this comment

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

LGTM but I would just remove run_once.rb, it seems unnecessary and we already have 2 ways to run benchmarks (run_benchmarks.rb and directly with ruby ... benchmark.rb)

Comment on lines +304 to +305
# Run once with YJIT stats
ruby --yjit-stats benchmarks/railsbench/benchmark.rb
Copy link
Member

@eregon eregon Dec 2, 2025

Choose a reason for hiding this comment

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

This doesn't run a single iteration so it's not "once", unlike the ./run_once.sh --yjit-stats benchmarks/railsbench/benchmark.rb before. Trivial to fix: #446 (comment)

# STACKPROF_OPTS='mode:object' MIN_BENCH_TIME=0 MIN_BENCH_ITRS=1 ruby -v -I harness-stackprof benchmarks/.../benchmark.rb
# STACKPROF_OPTS='mode:cpu,interval:10' MIN_BENCH_TIME=1 MIN_BENCH_ITRS=10 ruby -v -I harness-stackprof benchmarks/.../benchmark.rb
# STACKPROF_OPTS='mode:object' HARNESS=stackprof MIN_BENCH_TIME=0 MIN_BENCH_ITRS=1 ruby -v -I harness-stackprof benchmarks/.../benchmark.rb
# STACKPROF_OPTS='mode:cpu,interval:10' MIN_BENCH_ITRS=10 HARNESS=stackprof MIN_BENCH_TIME=1 MIN_BENCH_ITRS=10 ruby -v -I harness-stackprof benchmarks/.../benchmark.rb
Copy link
Member

@eregon eregon Dec 2, 2025

Choose a reason for hiding this comment

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

Suggested change
# STACKPROF_OPTS='mode:cpu,interval:10' MIN_BENCH_ITRS=10 HARNESS=stackprof MIN_BENCH_TIME=1 MIN_BENCH_ITRS=10 ruby -v -I harness-stackprof benchmarks/.../benchmark.rb
# STACKPROF_OPTS='mode:cpu,interval:10' HARNESS=stackprof MIN_BENCH_TIME=1 MIN_BENCH_ITRS=10 ruby -v -I harness-stackprof benchmarks/.../benchmark.rb

(MIN_BENCH_ITRS=10 is duplicated)

./run_once.sh --yjit-stats benchmarks/railsbench/benchmark.rb
```bash
# Run once with YJIT stats
ruby --yjit-stats benchmarks/railsbench/benchmark.rb
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ruby --yjit-stats benchmarks/railsbench/benchmark.rb
HARNESS=once ruby --yjit-stats benchmarks/railsbench/benchmark.rb

@eregon
Copy link
Member

eregon commented Dec 2, 2025

I can take a look at the test failures on truffleruby and push a commit to fix them if you'd like, let me know if so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants