-
Notifications
You must be signed in to change notification settings - Fork 117
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
Proper timestamp calculation for UUID v7 #199
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #199 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 5 5
Lines 545 548 +3
=========================================
+ Hits 545 548 +3 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you potentially accept a couple of benchmarks if I wrote them?
Sure! |
Edit: example results:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had a few comments and a suggestion but LGTM otherwise
@dylan-bourque Updated with your suggestions. |
Fixes #195.
The use of the
time.Time
methodUnixNanos()
was causing anint64
overflow (officially an "undefined behavior"), and returning incorrect timestamp values.There wasn't any good reason for using this function, since the goal is to convert milliseconds to 100-nanosecond intervals, which can be done much more simply with a single multiplication step.
This might also provide a slight performance improvement (see #128 (comment)), since
time.UnixMilli
, the (unneeded) call to.UTC()
, and.UnixNano()
are each doing a fair bit of work internally, and they have all been replaced with a single multiplication.