[gold] Reduce memory by not locking SearchIndex in warmer closure
After adding in the pre-slice maps, we were accidentally
keeping around all the traces in that map because
the search index was locked away in a closure when warming
Since warming takes 15 minutes or so and we refreshed a tile
after 5 minutes, we were keeping an extra 3-4 traces around.
This appears to help calm down the memory usage again.
I spent some time looking at bt_tracestore again, because
the memory blame blames things in loadEncodedTraces that
I would have thought would have been GC'd, but I now believe
that go does something clever when copying things around
that might make it look like they are still there but are not.
Additionally, we were incorrectly keeping the row keys around,
which can be prevented with a copy, just like in Perf.
Change-Id: I0e913330c10ddccc3cea4d794b2d447a8eb92ea1
Reviewed-on: https://skia-review.googlesource.com/c/buildbot/+/254800
Reviewed-by: Ben Wagner aka dogben <benjaminwagner@google.com>
Commit-Queue: Kevin Lubick <kjlubick@google.com>
diff --git a/go/util/util.go b/go/util/util.go
index 57c855c..8d750e5 100644
--- a/go/util/util.go
+++ b/go/util/util.go
@@ -499,6 +499,15 @@
return rv
}
+// CopyString returns a copy of the given string. This may seem unnecessary, but
+// is very important at preventing leaks of strings. For example, subslicing
+// a string can prevent the larger string from being cleaned up.
+func CopyString(s string) string {
+ b := &strings.Builder{}
+ b.WriteString(s)
+ return b.String()
+}
+
// KeysOfParamSet returns the keys of a param set.
func KeysOfParamSet(set map[string][]string) []string {
ret := make([]string, 0, len(set))
diff --git a/golden/go/indexer/indexer.go b/golden/go/indexer/indexer.go
index ffb283f..918f812 100644
--- a/golden/go/indexer/indexer.go
+++ b/golden/go/indexer/indexer.go
@@ -507,8 +507,8 @@
t := idx.cpxTile.GetTile(is)
for id, tr := range t.Traces {
gt, ok := tr.(*types.GoldenTrace)
- if !ok {
- sklog.Warningf("Unexpected trace type: %#v", gt)
+ if !ok || gt == nil {
+ sklog.Warningf("Unexpected trace type for id %s: %#v", id, gt)
continue
}
tp := types.TracePair{
@@ -619,7 +619,7 @@
byTest := idx.DigestCountsByTest(types.IncludeIgnoredTraces)
unavailableDigests, err := idx.diffStore.UnavailableDigests(ctx)
if err != nil {
- sklog.Warningf("could not fetch unavailables digests, going to assume all are valid: %s", err)
+ sklog.Warningf("could not fetch unavailable digests, going to assume all are valid: %s", err)
unavailableDigests = nil
}
// Collect all hashes in the tile that haven't been marked as unavailable yet.
@@ -661,18 +661,19 @@
}
d := digesttools.NewClosestDiffFinder(exp, idx.dCounters[is], idx.diffStore)
- go func() {
+ // Pass these in so as to allow the rest of the items in the index to be GC'd if needed.
+ go func(warmer warmer.DiffWarmer, summaries countsAndBlames, tests types.TestNameSet, dc digest_counter.DigestCounter, d digesttools.ClosestDiffFinder) {
// If there are somehow lots and lots of diffs or the warmer gets stuck, we should bail out
// at some point to prevent amount of work being done on the diffstore (e.g. a remote
// diffserver) from growing in an unbounded fashion.
// 15 minutes was chosen based on the 90th percentile time looking at the metrics.
ctx, cancel := context.WithTimeout(context.Background(), 15*time.Minute)
defer cancel()
- err := idx.warmer.PrecomputeDiffs(ctx, idx.summaries[is], idx.testNames, idx.dCounters[is], d)
- if err != nil {
- sklog.Warningf("Could not precompute diffs for %d summaries and %d test names: %s", len(idx.summaries[is]), len(idx.testNames), err)
+
+ if err := warmer.PrecomputeDiffs(ctx, summaries, tests, dc, d); err != nil {
+ sklog.Warningf("Could not precompute diffs for %d summaries and %d test names: %s", len(summaries), len(tests), err)
}
- }()
+ }(idx.warmer, idx.summaries[is], idx.testNames, idx.dCounters[is], d)
return nil
}
diff --git a/golden/go/tracestore/bt_tracestore/types.go b/golden/go/tracestore/bt_tracestore/types.go
index a75e010..ee8c5d9 100644
--- a/golden/go/tracestore/bt_tracestore/types.go
+++ b/golden/go/tracestore/bt_tracestore/types.go
@@ -306,7 +306,7 @@
if ok {
return d
}
- d = fromBytes(b)
+ d = fromBytes(k[:])
c[k] = d
return d
}
diff --git a/golden/go/tracestore/bt_tracestore/util.go b/golden/go/tracestore/bt_tracestore/util.go
index bb64712..dc17624 100644
--- a/golden/go/tracestore/bt_tracestore/util.go
+++ b/golden/go/tracestore/bt_tracestore/util.go
@@ -10,6 +10,7 @@
"go.skia.org/infra/go/paramtools"
"go.skia.org/infra/go/query"
"go.skia.org/infra/go/sklog"
+ "go.skia.org/infra/go/util"
"go.skia.org/infra/golden/go/tracestore"
"go.skia.org/infra/golden/go/types"
)
@@ -49,7 +50,10 @@
if len(parts) == 0 {
return ""
}
- return parts[len(parts)-1]
+ // We shouldn't return the result directly out of split, lest it
+ // leak the BigTable row (and potentially all the data associated with it).
+ // https://go101.org/article/memory-leaking.html
+ return util.CopyString(parts[len(parts)-1])
}
// toBytes turns a Digest into the bytes that will be stored in the table.
diff --git a/perf/go/btts/btts.go b/perf/go/btts/btts.go
index 913a196..77660ef 100644
--- a/perf/go/btts/btts.go
+++ b/perf/go/btts/btts.go
@@ -29,6 +29,7 @@
"go.skia.org/infra/go/skerr"
"go.skia.org/infra/go/sklog"
"go.skia.org/infra/go/timer"
+ "go.skia.org/infra/go/util"
"go.skia.org/infra/go/vec32"
"go.skia.org/infra/perf/go/btts/engine"
"go.skia.org/infra/perf/go/config"
@@ -1034,9 +1035,9 @@
g.Go(func() error {
return b.getTable().ReadRows(tctx, bigtable.PrefixRange(tileKey.TraceRowPrefix(i)), func(row bigtable.Row) bool {
parts := strings.Split(row.Key(), ":")
- b := &strings.Builder{}
- b.WriteString(parts[2])
- out <- b.String()
+ // prevent leaking the row data from the slice.
+ // https://go101.org/article/memory-leaking.html
+ out <- util.CopyString(parts[2])
return true
}, bigtable.RowFilter(
bigtable.ChainFilters(
