internal/core/adt: fix disjunction error counter issue

Because closeContext associated with holes are always clones in overlay.go for disjunctions, only the copies were ever decremented. This is not a big deal, as such counters never indicate an error: either they are replaced with a successful disjunction, or the node has no succeeding disjunctions and is already erroneous. To help balancing counters, however, we decrement the counters nonetheless. This may result in some extra postprocessing, which as said, is not strictly necessary. A TODO was added to consider disabling this block of code when DebugDeps is disabled. Signed-off-by: Marcel van Lohuizen <[email protected]> Change-Id: Ib28b7ed4c6b50677d6635c0247d0ca87c8e24537 Reviewed-on: https://cue.gerrithub.io/c/cue-lang/cue/+/1207716 TryBot-Result: CUEcueckoo <[email protected]> Unity-Result: CUE porcuepine <[email protected]> Reviewed-by: Matthew Sackman <[email protected]>
cue-lang · Jan 24, 2025 · 079442c · 079442c
1 parent f53f655
commit 079442c
Show file tree

Hide file tree

Showing 16 changed files with 71 additions and 48 deletions.
diff --git a/cue/testdata/benchmarks/cycleshare.txtar b/cue/testdata/benchmarks/cycleshare.txtar
@@ -33,7 +33,7 @@ Allocs: 64
 Retain: 0
 
 Unifications: 43
-Conjuncts:    168
+Conjuncts:    175
 Disjuncts:    21
 -- diff/-out/evalalpha/stats<==>+out/eval/stats --
 diff old new
@@ -55,7 +55,7 @@ diff old new
 -Conjuncts:    1046
 -Disjuncts:    976
 +Unifications: 43
-+Conjuncts:    168
++Conjuncts:    175
 +Disjuncts:    21
 -- out/eval/stats --
 Leaks:  8

diff --git a/cue/testdata/benchmarks/disjunctelim.txtar b/cue/testdata/benchmarks/disjunctelim.txtar
@@ -47,7 +47,7 @@ Allocs: 612
 Retain: 0
 
 Unifications: 22
-Conjuncts:    877
+Conjuncts:    954
 Disjuncts:    618
 -- diff/-out/evalalpha/stats<==>+out/eval/stats --
 diff old new
@@ -68,7 +68,7 @@ diff old new
 -Conjuncts:    866
 -Disjuncts:    367
 +Unifications: 22
-+Conjuncts:    877
++Conjuncts:    954
 +Disjuncts:    618
 -- out/eval/stats --
 Leaks:  0

diff --git a/cue/testdata/benchmarks/disjunction.txtar b/cue/testdata/benchmarks/disjunction.txtar
@@ -153,7 +153,7 @@ Allocs: 162
 Retain: 0
 
 Unifications: 6
-Conjuncts:    268
+Conjuncts:    308
 Disjuncts:    156
 -- diff/-out/evalalpha/stats<==>+out/eval/stats --
 diff old new
@@ -174,7 +174,7 @@ diff old new
 -Conjuncts:    143
 -Disjuncts:    82
 +Unifications: 6
-+Conjuncts:    268
++Conjuncts:    308
 +Disjuncts:    156
 -- out/eval/stats --
 Leaks:  0

diff --git a/cue/testdata/benchmarks/inlinedisjunction.txtar b/cue/testdata/benchmarks/inlinedisjunction.txtar
@@ -21,7 +21,7 @@ Allocs: 147
 Retain: 0
 
 Unifications: 17
-Conjuncts:    198
+Conjuncts:    218
 Disjuncts:    76
 -- out/eval --
 (struct){
@@ -60,7 +60,7 @@ diff old new
 -Conjuncts:    13409
 -Disjuncts:    4674
 +Unifications: 17
-+Conjuncts:    198
++Conjuncts:    218
 +Disjuncts:    76
 -- out/eval/stats --
 Leaks:  0

diff --git a/cue/testdata/benchmarks/issue1684.txtar b/cue/testdata/benchmarks/issue1684.txtar
@@ -69,7 +69,7 @@ Allocs: 1970
 Retain: 0
 
 Unifications: 445
-Conjuncts:    4476
+Conjuncts:    4775
 Disjuncts:    818
 -- out/evalalpha --
 (struct){
@@ -153,7 +153,7 @@ diff old new
 -Conjuncts:    2480117
 -Disjuncts:    1064333
 +Unifications: 445
-+Conjuncts:    4476
++Conjuncts:    4775
 +Disjuncts:    818
 -- diff/-out/evalalpha<==>+out/eval --
 diff old new

diff --git a/cue/testdata/benchmarks/listdedup.txtar b/cue/testdata/benchmarks/listdedup.txtar
@@ -40,7 +40,7 @@ Allocs: 168
 Retain: 0
 
 Unifications: 56
-Conjuncts:    471
+Conjuncts:    480
 Disjuncts:    40
 -- out/evalalpha --
 (struct){
@@ -555,7 +555,7 @@ diff old new
 -Conjuncts:    100730
 -Disjuncts:    24097
 +Unifications: 56
-+Conjuncts:    471
++Conjuncts:    480
 +Disjuncts:    40
 -- out/eval/stats --
 Leaks:  0

diff --git a/cue/testdata/benchmarks/listdisj.txtar b/cue/testdata/benchmarks/listdisj.txtar
@@ -62,7 +62,7 @@ Allocs: 16
 Retain: 0
 
 Unifications: 15
-Conjuncts:    43
+Conjuncts:    44
 Disjuncts:    4
 -- diff/-out/evalalpha/stats<==>+out/eval/stats --
 diff old new
@@ -83,7 +83,7 @@ diff old new
 -Conjuncts:    894
 -Disjuncts:    447
 +Unifications: 15
-+Conjuncts:    43
++Conjuncts:    44
 +Disjuncts:    4
 -- out/evalalpha --
 (struct){

diff --git a/cue/testdata/builtins/closed.txtar b/cue/testdata/builtins/closed.txtar
@@ -64,7 +64,7 @@ Allocs: 372
 Retain: 0
 
 Unifications: 179
-Conjuncts:    913
+Conjuncts:    922
 Disjuncts:    24
 -- diff/-out/evalalpha/stats<==>+out/eval/stats --
 diff old new
@@ -86,7 +86,7 @@ diff old new
 -Conjuncts:    389
 -Disjuncts:    204
 +Unifications: 179
-+Conjuncts:    913
++Conjuncts:    922
 +Disjuncts:    24
 -- out/eval/stats --
 Leaks:  43

diff --git a/cue/testdata/cycle/builtins.txtar b/cue/testdata/cycle/builtins.txtar
@@ -136,14 +136,14 @@ issue3633: final: {
 -- todo/p1 --
 issue3443.noCycle: fix hang
 -- out/evalalpha/stats --
-Leaks:  211
+Leaks:  227
 Freed:  17
 Reused: 17
-Allocs: 211
+Allocs: 227
 Retain: 0
 
-Unifications: 183
-Conjuncts:    696
+Unifications: 191
+Conjuncts:    726
 Disjuncts:    28
 -- diff/-out/evalalpha/stats<==>+out/eval/stats --
 diff old new
@@ -155,17 +155,17 @@ diff old new
 -Reused: 234
 -Allocs: 29
 -Retain: 70
-+Leaks:  211
++Leaks:  227
 +Freed:  17
 +Reused: 17
-+Allocs: 211
++Allocs: 227
 +Retain: 0
 
 -Unifications: 227
 -Conjuncts:    413
 -Disjuncts:    309
-+Unifications: 183
-+Conjuncts:    696
++Unifications: 191
++Conjuncts:    726
 +Disjuncts:    28
 -- out/eval/stats --
 Leaks:  14

diff --git a/cue/testdata/cycle/comprehension.txtar b/cue/testdata/cycle/comprehension.txtar
@@ -317,7 +317,7 @@ Allocs: 782
 Retain: 0
 
 Unifications: 502
-Conjuncts:    3030
+Conjuncts:    3097
 Disjuncts:    196
 -- out/evalalpha --
 Errors:
@@ -881,7 +881,7 @@ diff old new
 -Conjuncts:    2525
 -Disjuncts:    1404
 +Unifications: 502
-+Conjuncts:    3030
++Conjuncts:    3097
 +Disjuncts:    196
 -- out/eval/stats --
 Leaks:  50

diff --git a/cue/testdata/cycle/issue990.txtar b/cue/testdata/cycle/issue990.txtar
@@ -87,7 +87,7 @@ Allocs: 1182
 Retain: 0
 
 Unifications: 243
-Conjuncts:    4186
+Conjuncts:    4270
 Disjuncts:    204
 -- out/evalalpha --
 (struct){
@@ -338,7 +338,7 @@ diff old new
 -Conjuncts:    12056
 -Disjuncts:    3258
 +Unifications: 243
-+Conjuncts:    4186
++Conjuncts:    4270
 +Disjuncts:    204
 -- diff/-out/evalalpha<==>+out/eval --
 diff old new

diff --git a/cue/testdata/eval/conjuncts.txtar b/cue/testdata/eval/conjuncts.txtar
@@ -76,7 +76,7 @@ Allocs: 246
 Retain: 0
 
 Unifications: 43
-Conjuncts:    378
+Conjuncts:    426
 Disjuncts:    152
 -- out/evalalpha --
 Errors:
@@ -173,7 +173,7 @@ diff old new
 -Conjuncts:    135
 -Disjuncts:    86
 +Unifications: 43
-+Conjuncts:    378
++Conjuncts:    426
 +Disjuncts:    152
 -- diff/-out/evalalpha<==>+out/eval --
 diff old new

diff --git a/cue/testdata/eval/letjoin.txtar b/cue/testdata/eval/letjoin.txtar
@@ -139,7 +139,7 @@ Allocs: 203
 Retain: 0
 
 Unifications: 178
-Conjuncts:    1191
+Conjuncts:    1209
 Disjuncts:    36
 -- diff/-out/evalalpha/stats<==>+out/eval/stats --
 diff old new
@@ -161,7 +161,7 @@ diff old new
 -Conjuncts:    486
 -Disjuncts:    325
 +Unifications: 178
-+Conjuncts:    1191
++Conjuncts:    1209
 +Disjuncts:    36
 -- out/eval/stats --
 Leaks:  24

diff --git a/cue/testdata/eval/notify.txtar b/cue/testdata/eval/notify.txtar
@@ -1603,7 +1603,7 @@ Allocs: 2263
 Retain: 0
 
 Unifications: 760
-Conjuncts:    13284
+Conjuncts:    13707
 Disjuncts:    908
 -- diff/-out/evalalpha/stats<==>+out/eval/stats --
 diff old new
@@ -1625,7 +1625,7 @@ diff old new
 -Conjuncts:    150
 -Disjuncts:    111
 +Unifications: 760
-+Conjuncts:    13284
++Conjuncts:    13707
 +Disjuncts:    908
 -- out/eval/stats --
 Leaks:  34

diff --git a/internal/core/adt/disjunct2.go b/internal/core/adt/disjunct2.go
@@ -272,6 +272,39 @@ func (n *nodeContext) processDisjunctions() *Bottom {
 	a := n.disjunctions
 	n.disjunctions = n.disjunctions[:0]
 
+	holes := make([]disjunctHole, len(n.disjunctCCs))
+	copy(holes, n.disjunctCCs)
+
+	// Upon completion, decrement the DISJUNCT counters that were incremented
+	// in scheduleDisjunction. Note that this disjunction may be a copy of the
+	// original, in which case we need to decrement the copied disjunctCCs, not
+	// the original.
+	//
+	// This is not strictly necessary, but it helps for balancing counters.
+	// TODO: Consider disabling this when DebugDeps is not set.
+	defer func() {
+		// We add a "top" value to disable closedness checking for this
+		// disjunction to avoid a spurious "field not allowed" error.
+		// We return the errors below, which will, in turn, be reported as
+		// the error.
+		for i, d := range a {
+			// TODO(perf: prove that holeIDs are always stored in increasing
+			// order and allow for an incremental search to reduce cost.
+			for _, h := range holes {
+				if h.holeID != a[i].holeID {
+					continue
+				}
+				cc := h.cc
+				id := a[i].cloneID
+				id.cc = cc
+				c := MakeConjunct(d.env, top, id)
+				n.scheduleConjunct(c, d.cloneID)
+				cc.decDisjunct(n.ctx, DISJUNCT)
+				break
+			}
+		}
+	}()
+
 	if !initArcs(n.ctx, n.node) {
 		return n.getError()
 	}
@@ -308,16 +341,6 @@ func (n *nodeContext) processDisjunctions() *Bottom {
 		case 0:
 			// TODO: now we have disjunct counters, do we plug holes at all?
 
-			// We add a "top" value to disable closedness checking for this
-			// disjunction to avoid a spurious "field not allowed" error.
-			// We return the errors below, which will, in turn, be reported as
-			// the error.
-			// TODO: probably no longer needed:
-			for i++; i < len(a); i++ {
-				c := MakeConjunct(d.env, top, a[i].cloneID)
-				n.scheduleConjunct(c, d.cloneID)
-			}
-
 			// Empty intermediate result. Further processing will not result in
 			// any new result, so we can terminate here.
 			// TODO(errors): investigate remaining disjunctions for errors.

diff --git a/internal/core/adt/eval_test.go b/internal/core/adt/eval_test.go
@@ -76,12 +76,12 @@ var needFix = map[string]string{
 // counter errors.
 // TODO: These counters should all go to zero.
 var skipDebugDepErrors = map[string]int{
-	"cycle/disjunction":        4,
-	"cycle/structural":         7,
+	"cycle/disjunction":        1,
+	"cycle/structural":         5,
 	"disjunctions/errors":      3,
-	"disjunctions/elimination": 19,
+	"disjunctions/elimination": 18,
 	"disjunctions/nested":      1,
-	"eval/disjunctions":        3,
+	"eval/disjunctions":        1,
 	"eval/issue545":            1,
 	"eval/notify":              3,
 	"scalars/embed":            2,