-
Notifications
You must be signed in to change notification settings - Fork 1k
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
use syscall.Gettimeofday to get current time #961
Conversation
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.
This might be useful background info:
- https://tpaschalis.me/golang-time-now/
- https://pkg.go.dev/syscall#Gettimeofday
- https://man7.org/linux/man-pages/man2/gettimeofday.2.html
I think utils.Now()
should probably have some tests.
This doesn't seem to be an improvement on FreeBSD on Intel Atom.
But I can confirm the improvements on Linux on Intel Core Ultra 7
|
What I don't really know is if
|
According to https://tpaschalis.me/golang-time-now/, for linux amd64, when we call time.Now(), it uses runtime.vdsoClockgettimeSym, which makes a system call with context-switch overhead. But for freebsd, https://github.com/golang/go/blob/master/src/runtime/vdso_freebsd.go#L108 , time.Now prefers to use vdsoClockgettimeSym, and the same as linux. |
I agree with you that we should probably have some tests. |
This also fixes support for Windows:
|
utils/now.go
Outdated
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.
I would probably call this now_unix.go
and the other one now.go
or now_generic.go
or so.
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.
Good idea and I rename files name to now_unix.go and now.go
According to man doc: https://man7.org/linux/man-pages/man2/gettimeofday.2.html |
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.
rest lgtm
utils/now_unix.go
Outdated
// Now is a faster method to get current time | ||
func Now() time.Time { | ||
var tv syscall.Timeval | ||
if err := syscall.Gettimeofday(&tv); nil != err { |
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.
if err := syscall.Gettimeofday(&tv); nil != err { | |
if err := syscall.Gettimeofday(&tv); err != nil { |
nit. just a style preference. golang will not have if err = nil {}
problems
utils/now_unix_test.go
Outdated
customTimestamp := Now().UnixNano() | ||
|
||
// two timestamp should within 1 percistion | ||
assert.Equal(t, timestamp+int64(precision) >= customTimestamp && timestamp-int64(precision) <= customTimestamp, true, fmt.Sprintf("Loop: %d: customTimestamp should within %s. timestamp: %d, customTimestamp: %d", i, precision.String(), timestamp, customTimestamp)) |
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.
you can use assert.WithinRange
or assert.InDelta
utils/now_unix_test.go
Outdated
// two timestamp should within 1 percistion | ||
assert.Equal(t, timestamp+int64(precision) >= customTimestamp && timestamp-int64(precision) <= customTimestamp, true, fmt.Sprintf("Loop: %d: customTimestamp should within %s. timestamp: %d, customTimestamp: %d", i, precision.String(), timestamp, customTimestamp)) | ||
|
||
os.Setenv("TZ", fmt.Sprintf("UTC%d", 14-i%27)) |
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.
please add a comment for its purpose. I don't clearly understand
If we use syscall.Gettimeofday instead of time.Now, we could get a bit performance increasement.
Here are benchmark code and some result: