Closed nhubbard closed 1 year ago
@nhubbard Thank you for this contribution. Do you happen to have any comparative numbers concerning the performance of this solution with and without your changes?
@Theaninova @fvbakel, do you have any feedback on the changes this PR proposes? (@Theaninova: I'm tagging you because it seems your GitHub account committed the original solution, and wulkanat and bluefireoly don't seem to have a GitHub account at the moment.)
Hi, @wulkanat is actually me haha, I can't say too much other than seems good to me so far, it's been a hot minute since I touched Kotlin code.
Well, that's a gross deviation from the norm that @ handles on GitHub refer to GitHub account names, but I'll learn to live with that. :)
In any case, thanks for your feedback! I'll give the others some time to respond as well and wait for some performance numbers before I move forward with this PR.
Hi,
I have no time to look into this. I am also not really into Kotlin code anymore, so I am fine with whatever is the consensus is. Nice to see that there are still improvements😀
Kind regards,
Frank
Op zo 5 mrt. 2023 10:14 schreef Rutger van Bergen @.***
:
@nhubbard https://github.com/nhubbard Thank you for this contribution. Do you happen to have any comparative numbers concerning the performance of this solution with and without your changes?
@Theaninova https://github.com/Theaninova @fvbakel https://github.com/fvbakel, do you have any feedback on the changes this PR proposes? @.** https://github.com/Theaninova: I'm tagging you because it seems your GitHub account committed the original solution, and wulkanat and bluefireoly* don't seem to have a GitHub account at the moment.)
— Reply to this email directly, view it on GitHub https://github.com/PlummersSoftwareLLC/Primes/pull/897#issuecomment-1455033904, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEYYVDRZUUSVM7GV7FGPX6TW2RKPTANCNFSM6AAAAAAVO7BCIQ . You are receiving this because you were mentioned.Message ID: @.***>
Thanks for your feedback, Frank. That's clear.
I can definitely provide before and after performance numbers later, maybe tomorrow because I'm completely booked for the rest of the day.
I don't anticipate that much of an improvement all around, since most of my work was focused on adding Kotlin/Native coroutines support, but I might be pleasantly surprised!
Tomorrow is fine. It's mainly to make sure we don't unknowingly bring performance down in the process of updating.
Here's the benchmarks! All benchmarks were run 5 times in sequence on my 2021 MacBook Pro 16", with M1 Pro 10/16/16, 32 GB of 6400 MHz LPDDR5, and macOS Ventura 13.2.1.
Variant | HEAD Avg Score | PR Avg Score | Delta | Comments |
---|---|---|---|---|
JVM Traditional | 8047.2 | 8059 | 0.15% | |
JVM Idiomatic Fast | 8047.8 | 8095.4 | 0.59% | |
JVM Idiomatic | 6395.4 | 8090.2 | 26.50% | |
JVM Traditional SMP | 63871.6 | 64793.2 | 1.44% | |
JVM Idiomatic Fast SMP | 62991 | 65471.8 | 3.94% | |
JVM Idiomatic SMP | 48125.4 | 65814.8 | 36.76% | |
Native Traditional | 0 | 5899 | n/a | Unable to run HEAD version due to Gradle build errors |
Native Idiomatic Fast | 0 | 6700.6 | n/a | Unable to run HEAD version due to Gradle build errors |
Native Idiomatic | 0 | 6505.8 | n/a | Unable to run HEAD version due to Gradle build errors |
Native Traditional SMP | 0 | 47816 | n/a | Variant doesn't exist in HEAD version |
Native Idiomatic Fast SMP | 0 | 52504.2 | n/a | Variant doesn't exist in HEAD version |
Native Idiomatic SMP | 0 | 56223.6 | n/a | Variant doesn't exist in HEAD version |
JS Traditional | 2788 | 2793 | 0.18% | |
JS Idiomatic Fast | 2630.2 | 2734.4 | 3.96% | |
JS Idiomatic | 2806.8 | 2724.2 | -2.94% |
Description
This PR does the following:
Contributing requirements
drag-race
as the target branch.