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

Some improvements to the Rust version #2

Merged
merged 5 commits into from
Aug 17, 2024

Conversation

chancancode
Copy link
Contributor

@chancancode chancancode commented Aug 13, 2024

  • Fix clippy warnings
    These are just linter warnings about unidiomatic/unnecessary code, should be pretty harmless to apply.

  • Remove Node<T> generic
    This isn't supposed to change the compiled output (and at least on the stripped down version hosted on godbolt it didn't). Just making the Rust version match more 1:1 with the Zig version to remove some distractions.

  • Cleanup benchmark script
    The main thing is that the purpose of bench_with_input is to automatically wrap the input in black_box, so it was needlessly double-wrapped.

  • Make the two versions more identical
    The previous sum_rayon version has a extra check for (left && right) that made the code substantially different than the plain version, at least visually.

    This changes the implementations on both sides to match and also IMO a bit more idiomatic.

  • Make sum a method
    This matches the Zig version a bit closer in the source code, and is IMO more idiomatic, but it shouldn't really change the compiled output.

I did make some (lousy) effort to make sure none of these obviously caused any regression (it may be slightly faster?), but I was just running it on my laptop with a ton of browser tabs open, etc, etc. So probably should double check that in your benchmark environment.

These are just linter warnings about unidiomatic/unnecessary code,
should be pretty harmless to apply.
This isn't supposed to change the compiled output (and at least on
the stripped down version hosted on godbolt it didn't). Just making
the Rust version match more 1:1 with the Zig version to remove some
distractions.
The main thing is that the purpose of `bench_with_input` is to
automatically wrap the input in `black_box`, so it was needlessly
double-wrapped.
The previous sum_rayon version has a extra check for (left && right)
that made the code substantially different than the plain version,
at least visually.

This changes the implementations on both sides to match and also
IMO a bit more idiomatic.
This matches the Zig version a bit closer in the source code, and
is IMO more idiomatic, but it shouldn't really change the compiled
output.
@judofyr
Copy link
Owner

judofyr commented Aug 13, 2024

Oh nice! I was actually thinking of running it through clippy, but forgot about it. These changes look sensible to me. I’m a bit busy this week, but hopefully in a few days I’ll be able to re-do the benchmark in the same environment and verify that the results are similar.

@judofyr judofyr self-assigned this Aug 15, 2024
@judofyr judofyr added the accepted This will be worked on. label Aug 15, 2024
@judofyr
Copy link
Owner

judofyr commented Aug 17, 2024

I ran this again now and then I get the following diff:

# Rerun benchmark
(cd examples/rust-parallel-example && cargo bench)

# Regenerate CSV
rm bench/rayon-*.csv && make bench-rayon

# Diff
git diff
diff --git a/bench/rayon-tree-sum-1000.csv b/bench/rayon-tree-sum-1000.csv
index d9c615a..410a04e 100644
--- a/bench/rayon-tree-sum-1000.csv
+++ b/bench/rayon-tree-sum-1000.csv
@@ -1,7 +1,7 @@
-Baseline,1.5597891883229127
-Rayon 1 thread,23.51176644639185
-Rayon 2 threads,16.82203207978065
-Rayon 4 threads,14.939625885334602
-Rayon 8 threads,18.66874537774096
-Rayon 16 threads,24.183746144849145
-Rayon 32 threads,105.44453796411226
+Baseline,1.3574152155604338
+Rayon 1 thread,20.265803978522356
+Rayon 2 threads,15.612293062453892
+Rayon 4 threads,14.348052230794815
+Rayon 8 threads,16.42391594539397
+Rayon 16 threads,21.837912138899224
+Rayon 32 threads,96.13910653253916
diff --git a/bench/rayon-tree-sum-100M.csv b/bench/rayon-tree-sum-100M.csv
index b94de87..ef25279 100644
--- a/bench/rayon-tree-sum-100M.csv
+++ b/bench/rayon-tree-sum-100M.csv
@@ -1,7 +1,7 @@
-Baseline,7.4849190632000004
-Rayon 1 thread,22.9933083354
-Rayon 2 threads,11.7585642422
-Rayon 4 threads,5.898856567
-Rayon 8 threads,2.9989583844
-Rayon 16 threads,1.6433929402
-Rayon 32 threads,1.651134205
+Baseline,7.1566549926
+Rayon 1 thread,20.2334262024
+Rayon 2 threads,10.2145197118
+Rayon 4 threads,5.2211526282
+Rayon 8 threads,2.6398534263999998
+Rayon 16 threads,1.4336398069999998
+Rayon 32 threads,1.444313771

The machine appears to be slightly faster today (wonders of the cloud, maybe?), but the overall shape of the benchmark results still apply in this PR. Let's merge this!

@judofyr judofyr merged commit 1f55155 into judofyr:main Aug 17, 2024
1 check passed
@chancancode chancancode deleted the rust-fixes branch August 17, 2024 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted This will be worked on.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants