-
Notifications
You must be signed in to change notification settings - Fork 572
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
Update Crystal version to 1.14 and run code formatter #978
Conversation
@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. |
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. |
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. |
@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. |
The change is fine with me; I guess we need to update the compiler version occasionally. |
@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%
02b4405
to
c6edae3
Compare
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
Contributing requirements
drag-race
as the target branch.