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.

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

Adding the matmul benchmark from plb2 #526

Merged
merged 2 commits into from
Feb 14, 2025

Conversation

duffee
Copy link
Contributor

@duffee duffee commented Jan 21, 2025

Getting a start on #518. Adding matmul from the Programming Language Benchmark, I've created a tasks directory under examples/Benchmark for the scripts and included a guide to hold notes for hopeful benchmarkers.

If there's a better way of creating the matrices, let me know.

You can run a comparison with the Perl script using hyperfine

hyperfine --warmup 3 'path/to/matmul.pl 300' 'path/to/matrix_multiplication.pl 300'

I recommend low values at first because at N=1500, hyperfine takes over 10 minutes to measure the Perl script.

Copy link
Member

@mohawk2 mohawk2 left a comment

Choose a reason for hiding this comment

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

Please could you add the note you included in the comment here (about hyperfine) to the guide you made? And rename that guide to README.md, since that's effectively what it is?

@mohawk2
Copy link
Member

mohawk2 commented Jan 22, 2025

You'll note this is failing one of the release tests, which you can repro locally with prove -l xt. The solution is to run rm MANIFEST; make manifest

@mohawk2 mohawk2 force-pushed the master branch 2 times, most recently from 3d88e52 to a42733e Compare January 23, 2025 01:16
@duffee
Copy link
Contributor Author

duffee commented Jan 28, 2025

Adding notes to the new README.md:

I need a suggestion on how other devs can test two different versions of PDL on the same benchmark on the same command line. Different environment variables are a possibility, as are different paths. How do you work with 2 PDLs in the same git repo? Hyperfine takes the commands to run in single quotes, like so

hyperfine 'DEV_ENV=1 PERL5_LIB=over/here ./matrix_multiplication.pl' '...'

Thoughts?

Edit: hyperfine has --setup and --prepare options that allow you to run commands before timing runs so PDL could be re-compiled between comparisons.

@mohawk2
Copy link
Member

mohawk2 commented Jan 28, 2025

How do you work with 2 PDLs in the same git repo?

It's not really a "thing". Why would it be?

By the way, your most recent commit is a merge instead of a rebase. Please rebase and force-push, like the other times. Please make this part of your normal practice.

@mohawk2
Copy link
Member

mohawk2 commented Jan 28, 2025

You'll note this is failing one of the release tests, which you can repro locally with prove -l xt. The solution is to run rm MANIFEST; make manifest

This is still not fixed.

@duffee
Copy link
Contributor Author

duffee commented Jan 28, 2025

For 2 PDLs, I'm thinking of the case where you want to know if your code change has affected the benchmark.

I've added this to my git config (edited)

[pull]
      rebase = true

MANIFEST is fixed locally and will go up in the next commit. If you've no bright ideas, I'll push the README I've got so far.

@mohawk2
Copy link
Member

mohawk2 commented Jan 28, 2025

For 2 PDLs, I'm thinking of the case where you want to know if your code change has affected the benchmark.

In the normal way, I'd run the benchmark, make the change, run it again. But one can achieve similar with git checkout HEAD^ or any other ref. This is another use-case for git bisect, of course, to find where things changed. If Hyperfine is really taking 10 mins to speed-check a simple matmult, is it really worth using?

MANIFEST is fixed locally and will go up in the next commit. If you've no bright ideas, I'll push the README I've got so far.

My approach would be (and is) to push changes as I have them. Small, beneficial changes should be the "unit" of a commit.

@duffee duffee force-pushed the benchmarking-matmul branch from b4d65e1 to c9d5f5c Compare January 28, 2025 20:38
@duffee
Copy link
Contributor Author

duffee commented Jan 28, 2025

Requested changes made to MANIFEST and README.md

PR submitted to attractivechaos/plb2

@duffee duffee requested a review from mohawk2 January 28, 2025 21:56
@coveralls
Copy link

Coverage Status

coverage: 67.081%. remained the same
when pulling c9d5f5c on duffee:benchmarking-matmul
into a47768e on PDLPorters:master.

@mohawk2
Copy link
Member

mohawk2 commented Jan 29, 2025

This PR is now in much better shape, thank you. Your PR on plb2 looks good, too.

Are you planning to add more to this, or shall I merge it?

@duffee
Copy link
Contributor Author

duffee commented Jan 29, 2025

You can merge this. I'll come back to the other benchmarks after I've got my modules passing cpants again.

@mohawk2 mohawk2 merged commit 0151ae9 into PDLPorters:master Feb 14, 2025
83 checks passed
@duffee duffee deleted the benchmarking-matmul branch February 16, 2025 17:04
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.

3 participants