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
ddtrace/tracer: don't use UTC() when getting time #1134
Conversation
Unix time is defined as the duration since January 1, 1970 UTC. Calling .UTC() does unnecessary work to convert the "location" field of the time.Time object. The disassembly shows this new implementation is a few instructions shorter, but I doubt this will have any measurable performance impact. This is more about simplifying the existing code.
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.
Since you claim yourself that this doesn't have any measurable impact, why make this change and risk some unexpected regression?
It will save future people the time spent trying to figure out if If it helps, I could add the test I created that verifies that .UTC does not matter. It does not change the output of func TestUnixUTC(t *testing.T) {
now := time.Now()
unix := now.UnixNano()
unixUTC := now.UTC().UnixNano()
if unix != unixUTC {
t.Errorf("now=%#v unix=%#v != unixUTC=%#v", now, unix, unixUTC)
}
} |
@gbbr I might be wrong but if we remove |
@dianashevchenko I should add: I'm totally okay with this change being rejected. I just hate code that does nothing, and find this excess |
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're right. It makes sense. Thanks for explaining.
Unix time is defined as the duration since January 1, 1970 UTC.
Calling .UTC() does unnecessary work to convert the "location" field
of the time.Time object. The disassembly shows this new
implementation is a few instructions shorter, but I doubt this will
have any measurable performance impact. This is more about
simplifying the existing code.
I tried adding a test and benchmark for this, but the test was pretty silly, and it is unclear if the benchmark showed a measurable difference.