-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Memoize output of FileStore.Stats #7480
Conversation
@@ -258,6 +261,7 @@ func (f *FileStore) Add(files ...TSMFile) { | |||
for _, file := range files { | |||
atomic.AddInt64(&f.stats.DiskBytes, int64(file.Size())) | |||
} | |||
f.lastFileStats = nil // Will need to be recalculated on next call to Stats. |
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.
Lower down in the readers, I just resliced this to f.lastFileStats = f.lastFileStats[:0]
to avoid the extra allocations. Maybe do that here and check for len() == 0
?
@@ -449,6 +454,7 @@ func (f *FileStore) Close() error { | |||
file.Close() | |||
} | |||
|
|||
f.lastFileStats = nil |
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 also needs to be done in Replace
which is the primary way that files are added to the FileStore
via compactions.
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 one is in Close
so I think it's OK to set it to nil
.
@@ -598,6 +612,7 @@ func (f *FileStore) Replace(oldFiles, newFiles []string) error { | |||
// Tell the purger about our in-use files we need to remove | |||
f.purger.add(inuse) | |||
|
|||
f.lastFileStats = nil // Will need to be recalculated on next call to Stats. |
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.
[:0]
defer f.mu.RUnlock() | ||
if len(f.lastFileStats) > 0 { | ||
f.mu.RUnlock() | ||
return f.lastFileStats |
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 think you need to assign this under the read lock and return the assign var or else your accessing the slice outside the lock.
5c9ba86
to
72d716c
Compare
The rename in your test fails on windows/appveyor because the file is open. Probably need to do something like: https://github.com/influxdata/influxdb/blob/master/tsdb/engine/tsm1/compact_test.go#L2175 |
5fbf762
to
e002ffc
Compare
Required for all non-trivial PRs
This should significantly reduce the time spent planning compactions when there are large numbers of TSM files within a shard, by memoizing the output of
FileStore.Stats
. When new TSM files are added or removed the output ofFileStore.Stats
will need to be recalculated.Before:
After:
@jwilder @benbjohnson
/cc @beckettsean