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

fix: msecs option in v7 #768

Closed

Conversation

robinpokorny
Copy link
Contributor

This PR enables users to provide custom timestamp when generating v7. If fixes previous problems described in #764

  • Works when custom timestamp is the first call
  • Allows moving forward in time after previous call
  • Keeps monotonicity when supplied timestamp is same as previous or in the window

Implementation details

Previously we stored the previous timestamp together with any advancements. So we loose the information of the last supplied timestamp. I fixed it by storing the timestamp from which the advancements start, so we can determine if the new timestamp is in that window.

Also, seq reset did not work at all as the condition was reversed.

Benchmark

Main branch:

Starting. Tests take ~1 minute to run ...
uuid.stringify() x 4,443,136 ops/sec ±1.24% (93 runs sampled)
uuid.parse() x 4,184,593 ops/sec ±0.61% (98 runs sampled)
---

uuid.v1() x 4,169,435 ops/sec ±1.85% (96 runs sampled)
uuid.v1() fill existing array x 10,069,088 ops/sec ±0.15% (94 runs sampled)
uuid.v4() x 21,099,302 ops/sec ±1.78% (93 runs sampled)
uuid.v4() fill existing array x 7,462,047 ops/sec ±1.68% (92 runs sampled)
uuid.v4() without native generation x 3,318,972 ops/sec ±1.99% (93 runs sampled)
uuid.v3() x 552,987 ops/sec ±0.96% (94 runs sampled)
uuid.v5() x 646,356 ops/sec ±0.83% (92 runs sampled)
uuid.v6() x 2,447,460 ops/sec ±0.86% (96 runs sampled)
uuid.v7() x 2,865,804 ops/sec ±1.86% (95 runs sampled)
uuid.v7() fill existing array x 6,338,918 ops/sec ±0.50% (97 runs sampled)
uuid.v7() with defined time x 3,066,406 ops/sec ±0.47% (97 runs sampled)
Fastest is uuid.v4()
---

uuid.v1ToV6() x 2,305,596 ops/sec ±0.50% (97 runs sampled)
uuid.v1ToV6() w/ randomization x 2,294,125 ops/sec ±1.38% (98 runs sampled)
uuid.v6ToV1() x 2,005,151 ops/sec ±0.45% (98 runs sampled)

After:

Starting. Tests take ~1 minute to run ...
uuid.stringify() x 4,413,731 ops/sec ±0.62% (98 runs sampled)
uuid.parse() x 4,195,924 ops/sec ±0.53% (95 runs sampled)
---

uuid.v1() x 4,200,572 ops/sec ±0.70% (96 runs sampled)
uuid.v1() fill existing array x 10,071,859 ops/sec ±0.11% (96 runs sampled)
uuid.v4() x 20,864,060 ops/sec ±2.35% (93 runs sampled)
uuid.v4() fill existing array x 7,361,995 ops/sec ±1.12% (95 runs sampled)
uuid.v4() without native generation x 3,390,504 ops/sec ±1.82% (95 runs sampled)
uuid.v3() x 528,309 ops/sec ±0.59% (94 runs sampled)
uuid.v5() x 616,414 ops/sec ±1.83% (91 runs sampled)
uuid.v6() x 2,402,279 ops/sec ±0.49% (97 runs sampled)
uuid.v7() x 2,947,453 ops/sec ±0.53% (98 runs sampled)
uuid.v7() fill existing array x 6,305,512 ops/sec ±0.74% (95 runs sampled)
uuid.v7() with defined time x 3,049,547 ops/sec ±1.30% (97 runs sampled)
Fastest is uuid.v4()
---

uuid.v1ToV6() x 2,322,377 ops/sec ±0.24% (99 runs sampled)
uuid.v1ToV6() w/ randomization x 2,280,024 ops/sec ±1.09% (97 runs sampled)
uuid.v6ToV1() x 1,985,161 ops/sec ±0.38% (98 runs sampled)

@broofa broofa changed the title Fix msecs option in v7 fix: msecs option in v7 Jun 14, 2024
@broofa
Copy link
Member

broofa commented Jun 18, 2024

@robinpokorny Sorry for the delay on this. I'm kind of hoping @pmccarren will chime in here.

If this is very-urgent and pmccaren deems it appropriate we can push this out. But I'm not thrilled about adding yet-more internal state variables without being a bit more deliberate in how we handle them. I'm thinking it might be worth refactoring this code to formalize the internal state as a data structure that is used both internally or that can be optionally passed as an option. And then break the v6() method into two parts:

  1. update internal state
  2. generate uuid from internal state

That might be a breaking change though...? So maybe we push this out, then actually-fix how we handle internal state separately and maybe push that out with the breaking-changes in the TS port work.

@robinpokorny
Copy link
Contributor Author

@broofa, sound good!

Regarding the options, I was thinking there's one more. We can disable passing your own timestamp, for now, and only allow current time. For this use case it works quite well and I assume it's the most common one. The proper support for custom timestamp could be added later.

@broofa
Copy link
Member

broofa commented Jul 16, 2024

Moving this to #776

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.

2 participants