Skip to content
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

Avoid stat syscall when planning compactions #7335

Merged
merged 1 commit into from
Sep 26, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions tsdb/engine/tsm1/compact.go
Original file line number Diff line number Diff line change
Expand Up @@ -458,9 +458,8 @@ func (c *DefaultPlanner) Plan(lastWrite time.Time) []CompactionGroup {
// findGenerations groups all the TSM files by they generation based
// on their filename then returns the generations in descending order (newest first)
func (c *DefaultPlanner) findGenerations() tsmGenerations {
generations := map[int]*tsmGeneration{}

tsmStats := c.FileStore.Stats()
generations := make(map[int]*tsmGeneration, len(tsmStats))
for _, f := range tsmStats {
gen, _, _ := ParseTSMFileName(f.Path)

Expand Down
6 changes: 3 additions & 3 deletions tsdb/engine/tsm1/reader.go
Original file line number Diff line number Diff line change
Expand Up @@ -1174,9 +1174,9 @@ func (m *mmapAccessor) readAll(key string) ([]Value, error) {

func (m *mmapAccessor) path() string {
m.mu.RLock()
defer m.mu.RUnlock()

return m.f.Name()
path := m.f.Name()
m.mu.RUnlock()
return path
}

func (m *mmapAccessor) close() error {
Expand Down
54 changes: 38 additions & 16 deletions tsdb/engine/tsm1/tombstone.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,17 @@ const (
)

type Tombstoner struct {
mu sync.Mutex
mu sync.RWMutex

// 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think just caching the FileStat object as a value might be better

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm caching the slice because the interface returned is []FileStat Otherwise it allocates a slice each time.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// indicates that the stats may be out of sync with what is on disk and they
// should be refreshed.
statsLoaded bool
}

type Tombstone struct {
Expand Down Expand Up @@ -53,6 +59,8 @@ func (t *Tombstoner) AddRange(keys []string, min, max int64) error {
return nil
}

t.statsLoaded = false

tombstones, err := t.readTombstone()
if err != nil {
return nil
Expand All @@ -79,34 +87,48 @@ func (t *Tombstoner) Delete() error {
if err := os.RemoveAll(t.tombstonePath()); err != nil {
return err
}
t.statsLoaded = false
return nil
}

// HasTombstones return true if there are any tombstone entries recorded.
func (t *Tombstoner) HasTombstones() bool {
stat, err := os.Stat(t.tombstonePath())
if err != nil {
return false
}

return stat.Size() > 0
files := t.TombstoneFiles()
return len(files) > 0 && files[0].Size > 0
}

// TombstoneFiles returns any tombstone files associated with this TSM file.
func (t *Tombstoner) TombstoneFiles() []FileStat {
stat, err := os.Stat(t.tombstonePath())
if err != nil {
return nil
t.mu.RLock()
if t.statsLoaded {
stats := t.fileStats
t.mu.RUnlock()
return stats
}
t.mu.RUnlock()

if stat.Size() > 0 {
return []FileStat{FileStat{
Path: t.tombstonePath(),
LastModified: stat.ModTime().UnixNano(),
Size: uint32(stat.Size())}}
stat, err := os.Stat(t.tombstonePath())
if os.IsNotExist(err) || err != nil {
t.mu.Lock()
// The file doesn't exist so record that we tried to load it so
// we don't continue to keep trying. This is the common case.
t.statsLoaded = os.IsNotExist(err)
t.fileStats = t.fileStats[:0]
t.mu.Unlock()
return nil
}

return nil
t.mu.Lock()
t.fileStats = append(t.fileStats[:0], FileStat{
Path: t.tombstonePath(),
LastModified: stat.ModTime().UnixNano(),
Size: uint32(stat.Size()),
})
t.statsLoaded = true
stats := t.fileStats
t.mu.Unlock()

return stats
}

func (t *Tombstoner) Walk(fn func(t Tombstone) error) error {
Expand Down
33 changes: 33 additions & 0 deletions tsdb/engine/tsm1/tombstone_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,35 @@ func TestTombstoner_Add(t *testing.T) {
t.Fatalf("length mismatch: got %v, exp %v", got, exp)
}

stats := ts.TombstoneFiles()
if got, exp := len(stats), 0; got != exp {
t.Fatalf("stat length mismatch: got %v, exp %v", got, exp)
}

ts.Add([]string{"foo"})

entries, err = ts.ReadAll()
if err != nil {
fatal(t, "ReadAll", err)
}

stats = ts.TombstoneFiles()
if got, exp := len(stats), 1; got != exp {
t.Fatalf("stat length mismatch: got %v, exp %v", got, exp)
}

if stats[0].Size == 0 {
t.Fatalf("got size %v, exp > 0", stats[0].Size)
}

if stats[0].LastModified == 0 {
t.Fatalf("got lastModified %v, exp > 0", stats[0].LastModified)
}

if stats[0].Path == "" {
t.Fatalf("got path %v, exp != ''", stats[0].Path)
}

if got, exp := len(entries), 1; got != exp {
t.Fatalf("length mismatch: got %v, exp %v", got, exp)
}
Expand Down Expand Up @@ -83,6 +105,12 @@ func TestTombstoner_Add_Empty(t *testing.T) {
if got, exp := len(entries), 0; got != exp {
t.Fatalf("length mismatch: got %v, exp %v", got, exp)
}

stats := ts.TombstoneFiles()
if got, exp := len(stats), 0; got != exp {
t.Fatalf("stat length mismatch: got %v, exp %v", got, exp)
}

}

func TestTombstoner_Delete(t *testing.T) {
Expand Down Expand Up @@ -113,6 +141,11 @@ func TestTombstoner_Delete(t *testing.T) {
fatal(t, "delete tombstone", err)
}

stats := ts.TombstoneFiles()
if got, exp := len(stats), 0; got != exp {
t.Fatalf("stat length mismatch: got %v, exp %v", got, exp)
}

ts = &tsm1.Tombstoner{Path: f.Name()}
entries, err = ts.ReadAll()
if err != nil {
Expand Down