[all] Refactor time.Time to xtime.UnixNano #3515
Conversation
This PR refactors the usages of `time.Time` to `xtime.UnixNano` along the critical read path primarily, to reduce overhead from constant conversions to/from time.Times. Benchmarks show this is around a 13% improvement. On real-data benchmarks running `read_data_files`: With optimization: ``` Running time: 283.808698ms 35238 series read (124161.10 series/second) 2535668 datapoints decoded (8934426.67 datapoints/second) ``` Without optimization: ``` 35238 series read (109477.95 series/second) 2535668 datapoints decoded (7877851.54 datapoints/second) ``` Still TODO: update test files, get them running
…nikola/time-conversion
…nikola/time-conversion
Codecov Report
@@ Coverage Diff @@
## master #3515 +/- ##
======================================
Coverage 57.6% 57.6%
======================================
Files 549 549
Lines 67420 67420
======================================
Hits 38897 38897
Misses 25133 25133
Partials 3390 3390
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
src/dbnode/persist/fs/retriever.go
Outdated
| @@ -962,7 +967,7 @@ func (req *retrieveRequest) Clone( | |||
| } | |||
|
|
|||
| func (req *retrieveRequest) Start() time.Time { | |||
| return req.start | |||
| return req.start.ToTime() | |||
Maybe change to return UnixNano
Looks like this func is unused, removing it
src/dbnode/storage/block/block.go
Outdated
| @@ -475,13 +486,13 @@ func (dbb *databaseSeriesBlocks) Len() int { | |||
|
|
|||
| func (dbb *databaseSeriesBlocks) AddBlock(block DatabaseBlock) { | |||
| start := block.StartTime() | |||
| if dbb.min.Equal(timeZero) || start.Before(dbb.min) { | |||
| if dbb.min == timeZero || start < dbb.min { | |||
Maybe revert to the convenience methods
| @@ -1007,7 +1010,7 @@ func (s *fileSystemSource) bootstrapFromIndexPersistedBlocks( | |||
| s.log.Error("unable to read segments from index fileset", | |||
| zap.Stringer("namespace", ns.ID()), | |||
| zap.Error(err), | |||
| zap.Time("blockStart", indexBlockStart), | |||
| zap.Time("blockStart", indexBlockStart.ToTime()), | |||
Let's make sure we only do these conversions in error cases
Did a sweep; these conversions happen largely in errors/invariant violation logging, but are also called as part of the trace path for some large granularity functions like top level Query calls, where the overhead will be negligible (makes some sense since we wouldn't have been logging the hotpath anyway)
src/dbnode/storage/coldflush.go
Outdated
| @@ -107,7 +107,7 @@ func (m *coldFlushManager) Run(t time.Time) bool { | |||
| m.Unlock() | |||
| }() | |||
|
|
|||
| m.log.Debug("starting cold flush", zap.Time("time", t)) | |||
| m.log.Debug("starting cold flush", zap.Time("time", t.ToTime())) | |||
Maybe confirm how often this gets called
Seems like this may call decently often, going to put this behind a flag to only log/generate these fields if we're logging at debug level
What this PR does / why we need it:
This PR refactors the usages of
time.Timetoxtime.UnixNanoalong thecritical read path primarily, to reduce overhead from constant conversions
to/from time.Times.
Benchmarks show this is around a 13% improvement.
On real-data benchmarks running
read_data_files:With optimization:
Without optimization:
Special notes for your reviewer:
The most dangerous part of this conversion is in RPC conversions to ensure we maintain back/forward compat; be extra-critical around these areas
Still TODO: update test files, get them running
Does this PR introduce a user-facing and/or backwards incompatible change?:
If all the RPC conversions have been updated correctly, this should not be an issue. Any third party libraries may need updates, however.
The text was updated successfully, but these errors were encountered: