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

[Redbean] Add UuidV7 method #1213

Merged
merged 9 commits into from
Jul 1, 2024
Merged

[Redbean] Add UuidV7 method #1213

merged 9 commits into from
Jul 1, 2024

Conversation

mterron
Copy link
Contributor

@mterron mterron commented Jun 11, 2024

To Complete #1140 add UUID version 7 to Redbean

@github-actions github-actions bot added the tool label Jun 11, 2024
tool/net/lfuncs.c Outdated Show resolved Hide resolved
@mrdomino
Copy link
Collaborator

mrdomino commented Jun 16, 2024

On looking at the spec, it says that the first 48 bits of a UUIDv7 should be the timestamp in milliseconds since the UNIX epoch. The rest is allowed to be random data, but is also allowed to be a sub-millisecond timestamp or monotonic counter (to ensure sortability at sub-millisecond resolution.)

I'm a bit confused about all the conversions vs bit arithmetic at the moment, but it doesn't look like that's what the current implementation is doing - is it?

I would suggest instead taking, as your 64-bit value, the following:

uint64_t now_ms = now.tv_sec * 1000 + now.tv_nsec / 1000000;

Put (the lower 48 bits of) that in the first 48 bits. Then you have a choice; there are about (just shy of) 20 bits of further entropy in the tv_nsec field, and at least on my system the first (just shy of) 10 bits are supposed to be accurate for time ordering. So if you wanted, you could have the next 10–20 bits of the UUID come from the lower-order bits of that field.

But you don't have to; you could just fill the rest of the UUID after the first 48 bits with random data.

(I have questions about rounding with regard to the nanosecond field in my proposed implementation but I don't think those are as important right now.)

@mrdomino mrdomino removed the testing label Jun 16, 2024
@mterron
Copy link
Contributor Author

mterron commented Jun 16, 2024

On looking at the spec, it says that the first 48 bits of a UUIDv7 should be the timestamp in milliseconds since the UNIX epoch. The rest is allowed to be random data, but is also allowed to be a sub-millisecond timestamp or monotonic counter (to ensure sortability at sub-millisecond resolution.)

I'm a bit confused about all the conversions vs bit arithmetic at the moment, but it doesn't look like that's what the current implementation is doing - is it?

I would suggest instead taking, as your 64-bit value, the following:

uint64_t now_ms = now.tv_sec * 1000 + now.tv_nsec / 1000000;

Put (the lower 48 bits of) that in the first 48 bits. Then you have a choice; there are about (just shy of) 20 bits of further entropy in the tv_nsec field, and at least on my system the first (just shy of) 10 bits are supposed to be accurate for time ordering. So if you wanted, you could have the next 10–20 bits of the UUID come from the lower-order bits of that field.

But you don't have to; you could just fill the rest of the UUID after the first 48 bits with random data.

(I have questions about rounding with regard to the nanosecond field in my proposed implementation but I don't think those are as important right now.)

This is exactly what the implementation does actually. I have talked to Justine and have a less "magical" (or more explicit) implementation almost ready to push.

Thanks for the feedback! I won't try to be clever on my next commit.

@mrdomino
Copy link
Collaborator

Oh awesome, very good then. I dunno, cleverness is fine if it’s well documented, or if it’s clear enough that it doesn’t need to be documented.

Copy link
Owner

@jart jart left a comment

Choose a reason for hiding this comment

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

A few comments and requested changes, but overall LGTM.

tool/net/lfuncs.c Show resolved Hide resolved
tool/net/help.txt Outdated Show resolved Hide resolved
test/tool/net/uuidv7_test.lua Show resolved Hide resolved
@mterron mterron requested a review from jart June 21, 2024 09:04
Oops, forgot to change the 4 for a 7
Copy link
Collaborator

@mrdomino mrdomino left a comment

Choose a reason for hiding this comment

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

LGTM

Remember to run clang-format.

tool/net/lfuncs.c Show resolved Hide resolved
@mterron
Copy link
Contributor Author

mterron commented Jun 26, 2024

LGTM

Remember to run clang-format.

I can do that, it makes it really ugly and hard to follow IMO though

@mrdomino
Copy link
Collaborator

If it's giving you trouble, then my opinion would be maybe don't worry about it for now and it can get cleaned up later.

@mterron
Copy link
Contributor Author

mterron commented Jun 26, 2024

If it's giving you trouble, then my opinion would be maybe don't worry about it for now and it can get cleaned up later.

Let's keep it this way and merge it then

Copy link
Owner

@jart jart left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you!

@jart jart merged commit 72511ff into jart:master Jul 1, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants