internal/core/adt: fix benign memory leak
This fixes a memory leak that would cause a nodeContext
to be not recycled. As the nodeContext would have been
garbage collected anyway, this was just a matter of
performance.
A leak checker has been added to the tests. There are
some seemingly benign leaks remaining, so the test
uses Skipf for now.
Change-Id: I172a5a9f76e30a69cca398bd14f243ee13d96765
Reviewed-on: https://cue-review.googlesource.com/c/cue/+/8304
Reviewed-by: CUE cueckoo <cueckoo@gmail.com>
Reviewed-by: Marcel van Lohuizen <mpvl@golang.org>
diff --git a/internal/core/adt/disjunct.go b/internal/core/adt/disjunct.go
index 7f3475f..e7db3cd 100644
--- a/internal/core/adt/disjunct.go
+++ b/internal/core/adt/disjunct.go
@@ -330,6 +330,7 @@
// time that a clone is needed and must be nil. Conjuncts no longer needed and
// can become nil. All other fields can be copied shallowly.
func clone(v Vertex) Vertex {
+ v.state = nil
if a := v.Arcs; len(a) > 0 {
v.Arcs = make([]*Vertex, len(a))
for i, arc := range a {
diff --git a/internal/core/adt/eval.go b/internal/core/adt/eval.go
index b00b0b0..f873cf1 100644
--- a/internal/core/adt/eval.go
+++ b/internal/core/adt/eval.go
@@ -57,7 +57,18 @@
Allocs int
}
+// Leaks reports the number of nodeContext structs leaked. These are typically
+// benign, as they will just be garbage collected, as long as the pointer from
+// the original nodes has been eliminated or the original nodes are also not
+// referred to. But Leaks may have notable impact on performance, and thus
+// should be avoided.
+func (s *Stats) Leaks() int {
+ return s.Allocs + s.Reused - s.Freed
+}
+
var stats = template.Must(template.New("stats").Parse(`{{"" -}}
+
+Leaks: {{.Leaks}}
Freed: {{.Freed}}
Reused: {{.Reused}}
Allocs: {{.Allocs}}
@@ -330,10 +341,6 @@
}
n.expandDisjuncts(disState, n, maybeDefault, false)
- // If the state has changed, it is because a disjunct has been run. In this case, our node will have completed, and it will
- // set a value soon.
- v.state = n // alternatively, set to nil
-
for _, d := range n.disjuncts {
d.free()
}
@@ -373,6 +380,11 @@
// TODO: how to represent closedness information? Do we need it?
}
+ // If the state has changed, it is because a disjunct has been run, or
+ // because a single disjunct has replaced it. Restore the old state as
+ // to not confuse memory management.
+ v.state = n
+
if state != Finalized {
return
}
diff --git a/internal/core/adt/eval_test.go b/internal/core/adt/eval_test.go
index 017a021..2f422be 100644
--- a/internal/core/adt/eval_test.go
+++ b/internal/core/adt/eval_test.go
@@ -64,7 +64,11 @@
ctx := e.NewContext(v)
v.Finalize(ctx)
- t.Log(e.Stats())
+ stats := ctx.Stats()
+ t.Log(stats)
+ if n := stats.Leaks(); n > 0 {
+ t.Skipf("%d leaks reported", n)
+ }
if b := validate.Validate(ctx, v, &validate.Config{
AllErrors: true,
