Avoid stat syscall when planning compactions #7335
Conversation
|
@jwilder, thanks for your PR! By analyzing the annotation information on this pull request, we identified @joelegasse to be a potential reviewer |
jwilder
force-pushed
the
jw-tsm-syscalls
branch
4 times, most recently
from
5facad9 to
9a03791
Compare
| stat, err := os.Stat(t.tombstonePath()) | ||
| if err != nil { | ||
| if os.IsNotExist(err) { |
There was a problem hiding this comment.
this could be if os.IsNotExist(err) || err != nil { with t.statsLoaded = err == nil
There was a problem hiding this comment.
I think this needs to be t.stasLoaded = os.IsNotExist(err) since error will never be nil.
There was a problem hiding this comment.
That makes a lot more sense...
|
|
||
| // Path is the location of the file to record tombstone. This should be the | ||
| // full path to a TSM file. | ||
| Path string | ||
|
|
||
| // cache of the stats for this tombstone | ||
| fileStats []FileStat |
There was a problem hiding this comment.
I think just caching the FileStat object as a value might be better
There was a problem hiding this comment.
I'm caching the slice because the interface returned is []FileStat Otherwise it allocates a slice each time.
There was a problem hiding this comment.
There's still the concern of the data being modified, though. If we're okay with expecting the result to be used in a read-only fashion, then there are some other changes to allocate the slice exactly once: use t.fileStats = t.fileStats[:0] rather than assigning nil, and t.fileStats = append(t.fileStats[:0], FileStat{ ... instead of assigning a new slice.
There was a problem hiding this comment.
I'm not worried about it being modified, it's read-only. Re-using the slice is a better idea than setting it to nil too.
|
|
||
| return stat.Size() > 0 | ||
| t.mu.RLock() | ||
| b := len(t.fileStats) > 0 && t.fileStats[0].Size > 0 |
There was a problem hiding this comment.
This should probably validate that os.Stat was at least called once, possibly checking t.statsLoaded first, then falling back to calling TombstoneFiles (or ensuring the cache gets populated)
jwilder
force-pushed
the
jw-tsm-syscalls
branch
from
9a03791 to
1631034
Compare
|
@joelegasse Updated. |
| LastModified: stat.ModTime().UnixNano(), | ||
| Size: uint32(stat.Size())}} | ||
| t.mu.Lock() | ||
| stats := []FileStat{FileStat{ |
There was a problem hiding this comment.
in order to re-use the backing array, this should be t.fileStats = append(t.fileStats[:0], FileStat{
When the planner runs, it needs to determine if any files have tombstones. The code to determine if a tombstone existed involved stating the .tombstone file. Since the planner runs very frequently when there are many shards, this causea a lot of system calls that are unnecessary. Instead, cache the results of the stats calls and only refresh them when we haven't checked at least once or we write new tombstone data. This also caches the results of the TSMReader.Stats call to avoid creating garbage.
jwilder
force-pushed
the
jw-tsm-syscalls
branch
from
1631034 to
c2cfd63
Compare
|
@joelegasse Should be good now. |
Required for all non-trivial PRs
When the planner runs, it needs to determine if any files have tombstones.
The code to determine if a tombstone existed involved stating the .tombstone
file. Since the planner runs very frequently and when there are many shards, this
causes a lot of system calls that are unnecessary.
Instead, cache the results of the stats calls and only refresh them when we
haven't checked at least once or we write new tombstone data.
This also caches the results of the TSMReader.Stats call to avoid creating
garbage.