Skip to content

[Uid] fix performance and prevent collisions with the real clock_seq #41693

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 17, 2021
Merged

[Uid] fix performance and prevent collisions with the real clock_seq #41693

merged 1 commit into from
Jun 17, 2021

Conversation

nicolas-grekas
Copy link
Member

Q A
Branch? 5.3
Bug fix? no
New feature? no
Deprecations? no
Tickets -
License MIT
Doc PR -

Spotted while preparing my talk.

@carsonbot carsonbot added this to the 5.3 milestone Jun 14, 2021
@fancyweb
Copy link
Contributor

Do you have numbers to share? 😁

@nicolas-grekas
Copy link
Member Author

Yes, you need to see my talk :P

Nyholm
Nyholm approved these changes Jun 14, 2021
View reviewed changes
Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

What a way to promote your talk.

@nicolas-grekas nicolas-grekas force-pushed the uid-perf branch 3 times, most recently from 57e7a1f to bf75bd2 Compare June 14, 2021 15:14
@nicolas-grekas nicolas-grekas changed the title [Uid] fix performance [Uid] fix performance and prevent collisions with the real clock_seq Jun 14, 2021
nicolas-grekas
nicolas-grekas commented Jun 14, 2021
View reviewed changes
Copy link
Member Author

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Now with another bugfix: when a time is passed to generate a V1 or V6 UUID, the clock_seq is borrowed from the node if one is also set, or a static one is randomly generated otherwise.
This prevents any potential collisions with the real current clock seq.

@derrabus
Copy link
Member

For my understanding: The fix is that you prevent validating the same UUID/ULID twice when initializing them with fromString(), correct?

@nicolas-grekas
Copy link
Member Author

Absolutely!

@nicolas-grekas nicolas-grekas merged commit 9fb0fc9 into symfony:5.3 Jun 17, 2021
@fabpot fabpot mentioned this pull request Jun 17, 2021
Merged
@fancyweb fancyweb mentioned this pull request Jun 23, 2021
Merged
nicolas-grekas added a commit that referenced this pull request Jun 23, 2021
@nicolas-grekas
minor #41803 [Uid] Prevent double validation in Uuid::fromString() wi…
8a8d2a6 
…th base32 values (fancyweb)

This PR was merged into the 5.3 branch.

Discussion
----------

[Uid] Prevent double validation in Uuid::fromString() with base32 values

| Q             | A
| ------------- | ---
| Branch?       | 5.3
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

Follow up to #41693, it should be a little bit faster if we don't validate twice, right?

Commits
-------

043ee56 [Uid] Prevent double validation in Uuid::fromString() with base32 values
@nicolas-grekas nicolas-grekas deleted the uid-perf branch June 24, 2021 08:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Nyholm Nyholm Nyholm approved these changes

@derrabus derrabus derrabus approved these changes

Assignees
No one assigned
Labels
Status: Reviewed Uid
Projects
None yet
Milestone
5.3
Development

Successfully merging this pull request may close these issues.
5 participants
@nicolas-grekas @fancyweb @derrabus @Nyholm @carsonbot