ionspin / kotlin-multiplatform-bignum

A Kotlin multiplatform library for arbitrary precision arithmetics
Apache License 2.0
339 stars 40 forks source link

Improve performances of BigDecimal.numberOfDecimalDigits(). #299

Closed glureau closed 1 month ago

glureau commented 1 month ago

During a performance study on our app, we notice that numberOfDecimalDigits was taking a lot of time during our calculations.

image

I've made an adjustment and processed to a simple benchmark on a M3 Pro with this test

    @Test
    fun perfTest() {
        val bd = BigDecimal.parseString("1.0")
        val duration = measureTime {
            repeat(1_000_000) { bd.numberOfDecimalDigits() }
        }
        println("Duration: $duration")
    }

Initial results before the changes:

platform Run #1 Run #2 Run #3
iosSimulator 19.12s 17.95s 17.91s
JVM 536ms 538.7ms 559.1ms
macosArm64 18.2s 18.3s 18.1s

After the changes of the first commit, I obtain

platform Run #1 Run #2 Run #3 Comparison best of 3
iosSimulator 2.02s 2.01s 2.10s reduction of 88%
JVM 90.6ms 75.2ms 88.5ms reduction of 86%
macosArm64 2.10s 2.06s 2.07s reduction of 88%

Major changes:

CLAassistant commented 1 month ago

CLA assistant check
All committers have signed the CLA.

ionspin commented 1 month ago

Thank you for the great work! If you can sort out the CLA-Assistant we could get this meged! And don't worry about signing commits, that part I can skip.

glureau commented 1 month ago

Thank you! It would be wonderful if I can get a release with that change soon. 🙏🏻 Also I'm a bit confused, I've signed it but the bot still say it's not signed :(

Screenshot 2024-06-10 at 22 29 16
ionspin commented 1 month ago

I think the actual commit has to have the same email address you used to sign up with GitHub, I haven't read this article but I feel it's roughly in the direction of the solution :) https://docs.github.com/en/pull-requests/committing-changes-to-your-project/troubleshooting-commits/why-are-my-commits-linked-to-the-wrong-user

Oh and for the release, as soon as we merge it should be available in snapshot version.

glureau commented 1 month ago

It required a click on "recheck" 🤦🏻 CLA SIGNED ✅

glureau commented 1 month ago

Also I was expecting CI to run tests as I don't have the best config right now to run them. Should I do something to start them on CI?

EDIT: I'll prepare another PR for replacing all other == ZERO by isZero(), if it make sense to you.

ionspin commented 1 month ago

The CI requires manual approval for pull requests, so no worries I'll run that. As far as replacing == ZERO with isZero to be honest I can't answer right now, I'd have to go through the source and remind myself for that particular case. If you want to submit a pull request with that change, I think that would help have an overview of where it was used.

Also you have an extra bracket in the test that makes it not compile, I'll leave a comment to point it out.

ionspin commented 1 month ago

Merged, thanks once more! Once the CI is complete it will publish new snapshot version

ionspin commented 1 month ago

Deployed, your changes should be available in 0.3.10-SNAPSHOT https://oss.sonatype.org/content/repositories/snapshots/com/ionspin/kotlin/bignum/0.3.10-SNAPSHOT/