Skip to content

fix: msecs option in v7#768

Closed
robinpokorny wants to merge 3 commits intouuidjs:mainfrom
robinpokorny:fix-uuidv7-future-msecs
Closed

fix: msecs option in v7#768
robinpokorny wants to merge 3 commits intouuidjs:mainfrom
robinpokorny:fix-uuidv7-future-msecs

Conversation

@robinpokorny
Copy link
Copy Markdown
Contributor

@robinpokorny robinpokorny commented Jun 14, 2024

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)

@robinpokorny robinpokorny force-pushed the fix-uuidv7-future-msecs branch from 19c000e to 67b1217 Compare June 14, 2024 14:15
@broofa broofa changed the title Fix msecs option in v7 fix: msecs option in v7 Jun 14, 2024
@broofa
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

robinpokorny commented Jun 18, 2024

@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 broofa mentioned this pull request Jul 16, 2024
Closed
@broofa
Copy link
Copy Markdown
Member

broofa commented Jul 16, 2024

Moving this to #776

@broofa broofa closed this Jul 16, 2024
@broofa broofa mentioned this pull request Jul 17, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@robinpokorny @broofa