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

Update Crystal version to 1.14 and run code formatter #978

Merged

Conversation

Fryguy
Copy link
Contributor

@Fryguy Fryguy commented Oct 16, 2024

Description

Update Crystal version to 1.14 and run code formatter. Code formatting changes were required in order to avoid compiler warnings and do not change the existing logic.

Locally I'm seeing the following improvements

suite % improvement
marghidanu 12.2%
GordonBGood_bittwiddle 0.2%
GordonBGood_stride8 4.1%
GordonBGood_stride8-rblock16K -10.9%
GordonBGood_extreme 1.8%
GordonBGood_extreme-hybrid 8.0%

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.

@rbergen
Copy link
Contributor

rbergen commented Oct 17, 2024

@Fryguy Thanks for opening this.

Merging this on the first Crystal solution seems sensible to me because of the clear performance benefit it gives. However, that being a solution from another contributor (and another maintainer, as it happens to be), I will hereby tag @marghidanu to ask if he has objections against making this change to his solution.

When it comes to the second solution, things are less obvious. The reason of course being that a "-10.9%" improvement is not really an improvement. I'd therefore really like to have @GordonBGood's input on this proposed change.

@GordonBGood
Copy link
Contributor

@Fryguy Thanks for opening this.

Merging this on the first Crystal solution seems sensible to me because of the clear performance benefit it gives. However, that being a solution from another contributor (and another maintainer, as it happens to be), I will hereby tag @marghidanu to ask if he has objections against making this change to his solution.

When it comes to the second solution, things are less obvious. The reason of course being that a "-10.9%" improvement is not really an improvement. I'd therefore really like to have @GordonBGood's input on this proposed change.

I've scanned the differences of this commit as compared to my original code and see that indeed the changes are just to the code formatting. I attribute the differences in measured performance, whether better or worse, to likely the differences in the architecture on which the tests are being run and the differences in optimization with the newer version compiler. I have not confirmed the differences either on the machine on which the original work was done (which is not currently accessible to me) nor on my current faster machine as I have not been actively following the Crystal language development. I suspect that the single case where performance decreased is likely due to an order-of-access optimization/de-optimization. As this reflects the current state of Crystal development, I have no objections to the change being merged, but note that future versions of the Crystal compiler may reverse the single type of regression in performance so someone should continue to provide updates when meaningful.

Of course, the last statement applies to the compiler versions for all languages submissions if the results of the "Drag Race" are to be made current, and I must admit that I haven't followed up on updating my submissions for what I regard as the top contenders in the "Race" to the latest language version, which I regard to be Mike Barber's Rust versions, and my Nim, Haskell, Chapel, and maybe V language versions. The compilers for all of these have been updated since I submitted them and it is probable that newer compiler versions could slightly shift the relative performance. I'm reasonably sure that updating the Julia compiler version could also shift its relative rating.

Also, C and C++ have never had versions submitted that use the more extreme loop unrolling and dense culling techniques, which some work I did before I dropped this project indicated would make them competitive with the top contenders. I lost interest in the project because although the final results do somewhat indicate relative performance of groups of classes of languages, the small differences between members of a group depend on the test machine architecture, compiler version, and competence of the programmer soas not to be meaningful to real world use any more than top fuel dragster results to real world drivers. For instance, Kim Walich's very fast "primesieve" program uses pretty well all of the most extreme techniques used in our top submissions, yet these techniques only speed the counting of primes to smallish ranges of billions by fifteen percent or so - the limited range of only a million as per the requirements of the "Race" is a toy range in comparison which leads to the use of these techniques producing performance gains of 500 percent or more when compared to more conventional techniques.

@Fryguy
Copy link
Contributor Author

Fryguy commented Oct 17, 2024

As this reflects the current state of Crystal development, I have no objections to the change being merged

This was my sentiment as well and is why I opened the PR. Crystal 1.1 was released in 2021, and so much has improved in the compiler in that time. I intend to bring up this possible performance regression with the core team, but IMO people coming to this drag race to see the results want to see the most recent supported version.

@rbergen
Copy link
Contributor

rbergen commented Oct 18, 2024

IMO people coming to this drag race to see the results want to see the most recent supported version.

Personally I'm not sure this is a universal truth, but in light of @GordonBGood agreeing to the change that's irrelevant.

I'll give @marghidanu some more time to respond as well.

@rbergen
Copy link
Contributor

rbergen commented Oct 20, 2024

@Fryguy I've had some out-of-band alignment with @marghidanu, and he has approved the change you're proposing. However, we're now running into a CI snag with the solutions written in V - which were also submitted by @marghidanu and @GordonBGood, interestingly enough. That's because V has deprecated a language construct that's used in both V solutions.

@marghidanu has indicated he will look into that today, which should lead to a resolution of the problem. It will mean that you'll have to sync with PlummersSoftwareLLC:drag-race after his changes to the V solutions have been merged, to get CI to pass on this PR.

I'll comment here again when we're ready to move forward.

@marghidanu
Copy link
Contributor

The change is fine with me; I guess we need to update the compiler version occasionally.

@rbergen
Copy link
Contributor

rbergen commented Oct 20, 2024

@Fryguy The V issue was fixed by @marghidanu through #982. If you can sync your update_crystal_version branch with PlummersSoftwareLLC:drag-race, then we can proceed with merging this PR as well.

Locally I'm seeing the following improvements

suite                         | % improvement
----------------------------- | -------------
marghidanu                    |  12.2%
GordonBGood_bittwiddle        |   0.2%
GordonBGood_stride8           |   4.1%
GordonBGood_stride8-rblock16K | -10.9%
GordonBGood_extreme           |   1.8%
GordonBGood_extreme-hybrid    |   8.0%
@rbergen rbergen merged commit ad33b96 into PlummersSoftwareLLC:drag-race Oct 21, 2024
165 checks passed
@Fryguy Fryguy deleted the update_crystal_version branch October 22, 2024 21:27
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.

4 participants