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

✨ Faster uuid generation #2994

Merged
merged 5 commits into from
May 31, 2024
Merged

Conversation

ff137
Copy link
Contributor

@ff137 ff137 commented May 27, 2024

Next up on the list of improvements that nobody asked for :-) faster uuid generation!

I noticed in the StorageRecord class that a new uuid4 is always generated when one isn't provided. This seemed important. So I wondered what is the fastest way to generate a uuid, and unsurprisingly, there is a python-rust binding that claims to be 10x faster than the default python uuid library.

https://github.com/aminalaee/uuid-utils

I verified the benchmarks with a timeit script, comparing that with another rust-binding library fastuuid:

import timeit
import uuid

from fastuuid import uuid4 as fastuuid4
from uuid_utils import uuid4 as utiluuid4


def uuid4_hex():
    return uuid.uuid4().hex


def fastuuid4_hex():
    return fastuuid4().hex


def utiluuid4_hex():
    return utiluuid4().hex


if __name__ == "__main__":
    number = 1000000  # Number of executions for timeit

    uuid4_time = timeit.timeit("uuid4_hex()", globals=globals(), number=number)
    print(f"uuid4_hex().hex: {uuid4_time:.6f} seconds")

    fastuuid4_time = timeit.timeit("fastuuid4_hex()", globals=globals(), number=number)
    print(f"fastuuid4().hex: {fastuuid4_time:.6f} seconds")

    utiluuid4_hex_time = timeit.timeit(
        "utiluuid4_hex()", globals=globals(), number=number
    )
    print(f"utiluuid4_hex().hex: {utiluuid4_hex_time:.6f} seconds")

My results:

uuid4_hex().hex: 2.518658 seconds
fastuuid4().hex: 1.306156 seconds
utiluuid4_hex().hex: 0.280322 seconds

0.28 seconds vs 2.5 seconds. That's 9 StorageRecords that can now be generated in the time that it previously took for 1!

It's a drop-in replacement. If maintainers like it as well, maybe it'll make ACA-Py a lil bit faster :-)

jamshale
jamshale previously approved these changes May 28, 2024
Copy link
Contributor

@jamshale jamshale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a straightforward improvement 👍

Only possible concern would be that the library is owned by an individual instead of python, but I think the improvements are definitely worth switching.

@jamshale
Copy link
Contributor

jamshale commented May 30, 2024

FYI. Sonarcloud is working correctly now. I think it's just reporting stuff that already existed in the changed files. We should still fix or ignore issues if they aren't a lot of effort.

The integration test fail we can ignore. It's a flaky test that seems to be flakier recently.

@jamshale
Copy link
Contributor

@dbluhm @ianco Any opinions on this?

@jamshale jamshale requested review from dbluhm and ianco May 30, 2024 21:29
@ff137
Copy link
Contributor Author

ff137 commented May 30, 2024

not sure of the mean/variance for the integration tests, but it seems to consistently shave some time off

@jamshale
Copy link
Contributor

Recently merged a new workflow for the integration tests. The usually take just over 40mins and the latest run was a bit faster. They can vary a few minutes depending on if other integration tests are running at the same time. This latest run did seem a bit quicker though.

@dbluhm
Copy link
Member

dbluhm commented May 31, 2024

Seems like a low effort way to speed things up. If there ever is a problem with the library, seems like a find and replace across the project would really be all that was necessary to switch back so seems like pretty low risk to add in.

dbluhm
dbluhm previously approved these changes May 31, 2024
ff137 added 5 commits May 31, 2024 22:55
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
@ff137 ff137 dismissed stale reviews from dbluhm and jamshale via ef2e0ea May 31, 2024 19:56
@jamshale jamshale self-requested a review May 31, 2024 19:57
@jamshale jamshale enabled auto-merge (squash) May 31, 2024 19:59
@jamshale jamshale merged commit 304106a into hyperledger:main May 31, 2024
7 checks passed
Copy link

sonarcloud bot commented May 31, 2024

@ff137 ff137 deleted the feat/faster-uuid branch May 31, 2024 21:05
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.

3 participants