there is still a flake with adjust heights and height lower bound vs.…

… max height seen
wcharczuk · Feb 15, 2024 · 2feca44 · 2feca44
1 parent f55d73d
commit 2feca44
Show file tree

Hide file tree

Showing 2 changed files with 73 additions and 67 deletions.
diff --git a/adjust_heights_heap.go b/adjust_heights_heap.go
@@ -34,27 +34,77 @@ func (ah *adjustHeightsHeap) remove(node INode) {
 	ah.removeUnsafe(node)
 }
 
-func (ah *adjustHeightsHeap) removeUnsafe(node INode) {
-	if ah.numNodes == 0 {
-		return
+const heightUnset = -1
+
+func (ah *adjustHeightsHeap) setHeight(node INode, height int) error {
+	ah.mu.Lock()
+	defer ah.mu.Unlock()
+	return ah.setHeightUnsafe(node, height)
+}
+
+func (ah *adjustHeightsHeap) adjustHeights(rh *recomputeHeap, originalChild, originalParent INode) error {
+	ah.mu.Lock()
+	defer ah.mu.Unlock()
+
+	ah.heightLowerBound = originalChild.Node().height
+	if err := ah.ensureHeightRequirementUnsafe(originalChild, originalParent, originalChild, originalParent); err != nil {
+		return err
 	}
-	if node.Node().heightInAdjustHeightsHeap == heightUnset {
+	for ah.numNodes > 0 {
+		parent, _ := ah.removeMinUnsafe()
+		if parent.Node().heightInRecomputeHeap != heightUnset {
+			rh.fix(parent)
+		}
+		for _, child := range parent.Node().children {
+			if err := ah.ensureHeightRequirementUnsafe(originalChild, originalParent, child, parent); err != nil {
+				return err
+			}
+		}
+		if typed, typedOK := parent.(IBindChange); typedOK {
+			for _, nodeOnRight := range typed.RightScopeNodes() {
+				if nodeOnRight.Node().isNecessary() {
+					if err := ah.ensureHeightRequirementUnsafe(originalChild, originalParent, nodeOnRight, parent); err != nil {
+						return err
+					}
+				}
+			}
+		}
+	}
+	return nil
+}
+
+func (ah *adjustHeightsHeap) ensureHeightRequirementUnsafe(originalChild, originalParent, child, parent INode) error {
+	if originalParent.Node().id == child.Node().id {
+		return fmt.Errorf("cycle detected at %v to %v", originalChild, originalParent)
+	}
+	if parent.Node().height >= child.Node().height {
+		// we set `child.height` after adding `child` to the heap, so that `child` goes
+		// in the heap with its pre-adjusted height.
+		ah.addUnsafe(child)
+		if err := ah.setHeightUnsafe(child, parent.Node().height+1); err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
+func (ah *adjustHeightsHeap) removeMinUnsafe() (node INode, ok bool) {
+	if ah.numNodes == 0 {
 		return
 	}
-	nodeID := node.Node().id
-	height := node.Node().heightInAdjustHeightsHeap
-	if height >= ah.heightLowerBound && height <= ah.maxHeightSeen {
-		if ah.nodesByHeight[height] != nil && ah.nodesByHeight[height].has(nodeID) {
-			ah.nodesByHeight[height].remove(node.Node().id)
+	for x := ah.heightLowerBound; x <= ah.maxHeightSeen; x++ {
+		if ah.nodesByHeight[x] != nil && ah.nodesByHeight[x].len() > 0 {
+			_, node, ok = ah.nodesByHeight[x].pop()
+			ah.heightLowerBound = x
+			node.Node().heightInAdjustHeightsHeap = heightUnset
 			ah.numNodes--
 			return
 		}
 	}
+	return
 }
 
-const heightUnset = -1
-
-func (ah *adjustHeightsHeap) add(node INode) {
+func (ah *adjustHeightsHeap) addUnsafe(node INode) {
 	if node.Node().heightInAdjustHeightsHeap != heightUnset {
 		return
 	}
@@ -70,23 +120,25 @@ func (ah *adjustHeightsHeap) add(node INode) {
 	ah.numNodes++
 }
 
-func (ah *adjustHeightsHeap) removeMin() (node INode, ok bool) {
+func (ah *adjustHeightsHeap) removeUnsafe(node INode) {
 	if ah.numNodes == 0 {
 		return
 	}
-	for x := ah.heightLowerBound; x <= ah.maxHeightSeen; x++ {
-		if ah.nodesByHeight[x] != nil && ah.nodesByHeight[x].len() > 0 {
-			_, node, ok = ah.nodesByHeight[x].pop()
-			ah.heightLowerBound = x
-			node.Node().heightInAdjustHeightsHeap = heightUnset
+	if node.Node().heightInAdjustHeightsHeap == heightUnset {
+		return
+	}
+	nodeID := node.Node().id
+	height := node.Node().heightInAdjustHeightsHeap
+	if height >= ah.heightLowerBound && height <= ah.maxHeightSeen {
+		if ah.nodesByHeight[height] != nil && ah.nodesByHeight[height].has(nodeID) {
+			ah.nodesByHeight[height].remove(node.Node().id)
 			ah.numNodes--
 			return
 		}
 	}
-	return
 }
 
-func (ah *adjustHeightsHeap) setHeight(node INode, height int) error {
+func (ah *adjustHeightsHeap) setHeightUnsafe(node INode, height int) error {
 	if height > ah.maxHeightAllowed() {
 		return fmt.Errorf("cannot set node height above %d", ah.maxHeightAllowed())
 	}
@@ -96,49 +148,3 @@ func (ah *adjustHeightsHeap) setHeight(node INode, height int) error {
 	node.Node().height = height
 	return nil
 }
-
-func (ah *adjustHeightsHeap) ensureHeightRequirement(originalChild, originalParent, child, parent INode) error {
-	if originalParent.Node().id == child.Node().id {
-		return fmt.Errorf("cycle detected at %v to %v", originalChild, originalParent)
-	}
-	if parent.Node().height >= child.Node().height {
-		// we set `child.height` after adding `child` to the heap, so that `child` goes
-		// in the heap with its pre-adjusted height.
-		ah.add(child)
-		if err := ah.setHeight(child, parent.Node().height+1); err != nil {
-			return err
-		}
-	}
-	return nil
-}
-
-func (ah *adjustHeightsHeap) adjustHeights(rh *recomputeHeap, originalChild, originalParent INode) error {
-	ah.mu.Lock()
-	defer ah.mu.Unlock()
-
-	ah.heightLowerBound = originalChild.Node().height
-	if err := ah.ensureHeightRequirement(originalChild, originalParent, originalChild, originalParent); err != nil {
-		return err
-	}
-	for ah.numNodes > 0 {
-		parent, _ := ah.removeMin()
-		if parent.Node().heightInRecomputeHeap != heightUnset {
-			rh.fix(parent)
-		}
-		for _, child := range parent.Node().children {
-			if err := ah.ensureHeightRequirement(originalChild, originalParent, child, parent); err != nil {
-				return err
-			}
-		}
-		if typed, typedOK := parent.(IBindChange); typedOK {
-			for _, nodeOnRight := range typed.RightScopeNodes() {
-				if nodeOnRight.Node().isNecessary() {
-					if err := ah.ensureHeightRequirement(originalChild, originalParent, nodeOnRight, parent); err != nil {
-						return err
-					}
-				}
-			}
-		}
-	}
-	return nil
-}
diff --git a/graph_test.go b/graph_test.go
@@ -94,7 +94,7 @@ func Test_Graph_removeNodeFromGraph(t *testing.T) {
 		func(_ context.Context) {},
 	}
 	g.recomputeHeap.add(mn00)
-	g.adjustHeightsHeap.add(mn00)
+	g.adjustHeightsHeap.addUnsafe(mn00)
 
 	g.removeNode(mn00)