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

Switch from std::sync::mpsc to crossbeam-channel #1146

Merged
merged 2 commits into from
Nov 1, 2022

Conversation

tavianator
Copy link
Collaborator

  • walk: Switch back to crossbeam-channel
  • walk: Use a bounded queue.

@tavianator
Copy link
Collaborator Author

This is about a 5% perf improvement for me. @sharkdp Do you still experience the perf regression from #895 (comment)? If so, can you send me the perf.data files from a perf record run before and after this change?

@sharkdp
Copy link
Owner

sharkdp commented Oct 24, 2022

I currently don't have access to my laptop, but will get some benchmark results next week. Thank you!

This would also resolve the race condition panic bug with the std Rust channels, correct?

@tavianator
Copy link
Collaborator Author

I currently don't have access to my laptop, but will get some benchmark results next week. Thank you!

Great! perf stat output would also be helpful.

By the way, if you're taking these results on a laptop, make sure you set the scaling governor to performance (sudo cpupower frequency-set -g performance). It's possible that different frequency scaling behaviour explains the regression you observed. My laptop, for example, runs the bfs tests faster when there's 4 simultaneous copies, since it scales up more aggressively:

tavianator@graphene $ time ./tests/tests.sh
./tests/tests.sh  3.72s user 0.78s system 114% cpu 3.951 total
tavianator@graphene $ time parallel -u -N0 ./tests/tests.sh ::: {1..4}
parallel -u -N0 ./tests/tests.sh ::: {1..4}  6.56s user 0.85s system 321% cpu 2.306 total

But the performance governor makes it make sense:

tavianator@graphene $ sudo cpupower frequency-set -g performance
tavianator@graphene $ time ./tests/tests.sh
./tests/tests.sh  1.55s user 0.18s system 109% cpu 1.580 total
tavianator@graphene $ time parallel -u -N0 ./tests/tests.sh ::: {1..4}
parallel -u -N0 ./tests/tests.sh ::: {1..4}  6.46s user 0.80s system 323% cpu 2.244 total

This would also resolve the race condition panic bug with the std Rust channels, correct?

Yes, the first commit should fix #1060/#1113.

@sharkdp
Copy link
Owner

sharkdp commented Oct 31, 2022

Here are the results. I compared master (4257034) with master + the two commits in this branch cherry-picked. Scaling governor was set to "performance" (see also: sharkdp/hyperfine#239).

The good news is that I see no significant performance difference in the "simple pattern" benchmark anymore! However, I do see a consistent 20% regression in the "interactive output" benchmark. I can try to play with the channel size later on, or add perf results. Can anyone else reproduce this?

fd regression benchmark

Interactive output

Command Mean [s] Min [s] Max [s] Relative
./fd-master '' '/home/shark/Informatik/' 1.570 ± 0.053 1.504 1.644 1.00
./fd-crossbeam '' '/home/shark/Informatik/' 1.861 ± 0.015 1.837 1.884 1.19 ± 0.04

No pattern

Command Mean [ms] Min [ms] Max [ms] Relative
./fd-master --hidden --no-ignore '' '/home/shark/Informatik/' 388.7 ± 6.2 380.8 402.2 1.00 ± 0.02
./fd-crossbeam --hidden --no-ignore '' '/home/shark/Informatik/' 386.8 ± 3.1 382.1 390.5 1.00

Simple pattern

Command Mean [ms] Min [ms] Max [ms] Relative
./fd-master '.*[0-9]\.jpg$' '/home/shark/Informatik/' 172.8 ± 2.1 170.3 180.6 1.00
./fd-crossbeam '.*[0-9]\.jpg$' '/home/shark/Informatik/' 173.5 ± 1.4 171.5 177.2 1.00 ± 0.01

Simple pattern (-HI)

Command Mean [ms] Min [ms] Max [ms] Relative
./fd-master -HI '.*[0-9]\.jpg$' '/home/shark/Informatik/' 350.9 ± 2.8 347.0 355.8 1.00
./fd-crossbeam -HI '.*[0-9]\.jpg$' '/home/shark/Informatik/' 351.7 ± 2.7 346.1 355.6 1.00 ± 0.01

File extension

Command Mean [ms] Min [ms] Max [ms] Relative
./fd-master -HI --extension jpg '' '/home/shark/Informatik/' 370.8 ± 3.6 365.4 375.9 1.00
./fd-crossbeam -HI --extension jpg '' '/home/shark/Informatik/' 371.2 ± 7.7 365.8 392.1 1.00 ± 0.02

File type

Command Mean [ms] Min [ms] Max [ms] Relative
./fd-master -HI --type l '' '/home/shark/Informatik/' 351.4 ± 3.1 346.8 355.9 1.00 ± 0.01
./fd-crossbeam -HI --type l '' '/home/shark/Informatik/' 350.6 ± 3.1 347.4 358.5 1.00

Command execution

Command Mean [ms] Min [ms] Max [ms] Relative
./fd-master 'ab' '/home/shark/Informatik/' --exec echo 398.7 ± 1.7 396.1 401.3 1.00
./fd-crossbeam 'ab' '/home/shark/Informatik/' --exec echo 403.9 ± 2.4 400.1 407.3 1.01 ± 0.01

Command execution (large output)

Command Mean [ms] Min [ms] Max [ms] Relative
./fd-master -tf 'ab' '/home/shark/Informatik/' --exec cat 353.5 ± 5.5 349.4 368.4 1.00
./fd-crossbeam -tf 'ab' '/home/shark/Informatik/' --exec cat 355.7 ± 2.2 351.5 359.9 1.01 ± 0.02

Cold cache

Command Mean [s] Min [s] Max [s] Relative
./fd-master -HI '.*[0-9]\.jpg$' '/home/shark/Informatik/' 3.036 ± 0.051 3.000 3.094 1.01 ± 0.02
./fd-crossbeam -HI '.*[0-9]\.jpg$' '/home/shark/Informatik/' 3.018 ± 0.041 2.992 3.065 1.00

@tavianator
Copy link
Collaborator Author

Indeed, that one I can reproduce:

Command Mean [ms] Min [ms] Max [ms] Relative
./fd-master '' '/home/tavianator/code/llvm/llvm-project' 686.5 ± 27.0 653.7 739.0 1.00
./fd-feature '' '/home/tavianator/code/llvm/llvm-project' 783.2 ± 20.9 752.8 820.0 1.14 ± 0.05

@sharkdp
Copy link
Owner

sharkdp commented Oct 31, 2022

Increasing the channel size helps. The performance with a size of 2^14=16384 or higher seems to be similar to master.

image

Are there any drawbacks in doing so by default (higher memory usage)?

@tavianator
Copy link
Collaborator Author

Are there any drawbacks in doing so by default (higher memory usage)?

Yeah just higher memory usage. 16k * nthreads seems fine to me though.

Copy link
Owner

@sharkdp sharkdp left a comment

Choose a reason for hiding this comment

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

Awesome. Then let's increase the channel size and finally switch to crossbeam. Really looking forward to seeing all those tickets closed. Thank you 👍

@tavianator tavianator marked this pull request as ready for review November 1, 2022 13:53
@tavianator
Copy link
Collaborator Author

I bumped the channel size, should be good to go

@tmccombs
Copy link
Collaborator

tmccombs commented Nov 1, 2022

once the conflicts are resolved anyway :)

@sharkdp sharkdp merged commit 5278405 into sharkdp:master Nov 1, 2022
@sharkdp
Copy link
Owner

sharkdp commented Nov 1, 2022

Thank you for the update.

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