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

Use while loop instead of @simd for loop for PrimeJulia/solution_3 #584

Merged

Conversation

louie-github
Copy link
Contributor

Description

This PR is a bit unusual in terms of performance. On my Core i5 9th Gen machine, this hurts the performance a little bit:

❯ julia primes_1of2.jl && julia primes_edited.jl
Settings: sieve_size = 1000000 | duration = 5
Number of trues: 78498
primes_1of2.jl: Passes: 9609 | Elapsed: 5.000175952911377 | Passes per second: 1921.7323731188126 | Average pass duration: 0.0005203638206797145
louie-github_port_1of2;9609;5.000175952911377;1;algorithm=base,faithful=yes,bits=1
Settings: sieve_size = 1000000 | duration = 5
Number of trues: 78498
primes_1of2.jl: Passes: 9515 | Elapsed: 5.000458002090454 | Passes per second: 1902.8257003702922 | Average pass duration: 0.0005255342093631586
louie-github_port_1of2;9515;5.000458002090454;1;algorithm=base,faithful=yes,bits=1

However, on the Raspberry Pi, it is much faster because of the lack of @simd:

❯ julia primes_1of2.jl && julia primes_edited.jl
Settings: sieve_size = 1000000 | duration = 5
Number of trues: 78498
primes_1of2.jl: Passes: 870 | Elapsed: 5.0015950202941895 | Passes per second: 173.94451099498002 | Average pass duration: 0.005748959793441597
louie-github_port_1of2;870;5.0015950202941895;1;algorithm=base,faithful=yes,bits=1
Settings: sieve_size = 1000000 | duration = 5
Number of trues: 78498
primes_1of2.jl: Passes: 1149 | Elapsed: 5.001935958862305 | Passes per second: 229.71105776838877 | Average pass duration: 0.004353295003361449
louie-github_port_1of2;1149;5.001935958862305;1;algorithm=base,faithful=yes,bits=1

I think it is worth the slight performance hit (at least, until I find more optimizations) to make the results more consistent across machines with different CPUs and different CPU architectures.

Contributing requirements

  • I read the contribution guidelines in CONTRIBUTING.md.
  • I placed my solution in the correct solution folder.
  • I added a README.md with the right badge(s).
  • I added a Dockerfile that builds and runs my solution.
  • I selected drag-race as the target branch.
  • All code herein is licensed compatible with BSD-3.

@louie-github
Copy link
Contributor Author

louie-github commented Aug 4, 2021

On a slightly unrelated note, I plan to majorly refactor this script into a Julia "package" structure so that it would be easier to add new implementations in the future. This would require renaming and splitting the main primes_1of2.jl file, which I understand might be difficult to diff, as you mentioned in #510 concerning their plans to restructure main.rs. However, I am prepared to do what it takes to make it easier for you to process these changes, as I feel that this change is really necessary to make adding newer implementations much easier.

In short, I plan to separate each implementation similar to what PrimeRust/solution_1 has done, but into separate files, with a lot of the functionality being abstracted away. I think this would be a bit easier on my solution since it is a much smaller file, but once again, I am prepared to make it easier for you to review this in any way possible.

@rbergen
Copy link
Contributor

rbergen commented Aug 4, 2021

In general terms and speaking for myself:

  1. if a change isn't needed, I prefer not to see it
  2. if a change is needed, smaller changes are better from a review perspective than bigger ones
  3. fewer (slightly) bigger PRs are better than more smaller PRs

Points 2 and 3 may seem somewhat contradictory, and which one is most true does depend on the situation.

All that said, if a bigger (refactor) change is expected to make future changes smaller/easier to review, then it can be considered a good investment.

Then concerning your intended refactor: I don't know exactly what you're planning to do during it, or what you're planning to do after it. That makes it really hard to tell you now if I think it's a good or a bad move. So I guess that in the end, you'll have to rely most on your own judgment.
And of course, if you want to see what "review damage" you're inflicting, you can always ask GitHub to do a comparison between your fork branch and this repo. It'll tell you exactly what we need to look at if you open your PR. :)

@rbergen rbergen merged commit b6c5096 into PlummersSoftwareLLC:drag-race Aug 4, 2021
@louie-github louie-github deleted the julia3-while-loop branch August 5, 2021 15:33
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.

2 participants