From 21420d8b0976dc034feb90ab2878ae0dd63121ae Mon Sep 17 00:00:00 2001
From: ringabout <43030857+ringabout@users.noreply.github.com>
Date: Wed, 13 Nov 2024 05:57:31 +0800
Subject: [PATCH] fixes #24402; Memory leak under Arc/Orc on inline iterators
 with nested seq (#24419)

fixes #24402

```nim
iterator myPairsInline*[T](twoDarray: seq[seq[T]]): (int, seq[T]) {.inline.} =
  for indexValuePair in twoDarray.pairs:
    yield indexValuePair

proc innerTestTotalMem() =
  var my2dArray: seq[seq[int32]] = @[]

  # fill with some data...
  for i in 0'i32..100:
    var z = @[i, i+1]
    my2dArray.add z

  for oneDindex, innerArray in myPairsInline(my2dArray):
    discard

innerTestTotalMem()
```

In `for oneDindex, innerArray in myPairsInline(my2dArray)`, `oneDindex`
and `innerArray` becomes `cursors` because they satisfy the criterion of
`isSimpleIteratorVar`. On the one hand, it is not correct to have them
point to the temporary generated by tuple unpacking, which left the
memory of the temporary uncleaned up. On the other hand, we don't need
to generate a temporary for a symbol node when unpacking the tuple.
---
 compiler/transf.nim  | 35 +++++++++++++++-------------
 tests/arc/t24402.nim | 54 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 73 insertions(+), 16 deletions(-)
 create mode 100644 tests/arc/t24402.nim

diff --git a/compiler/transf.nim b/compiler/transf.nim
index 59e4453fd7..5cf43e6a3f 100644
--- a/compiler/transf.nim
+++ b/compiler/transf.nim
@@ -366,6 +366,19 @@ proc transformAsgn(c: PTransf, n: PNode): PNode =
   result[0] = letSection
   result[1] = asgnNode
 
+template assignTupleUnpacking(c: PTransf, e: PNode) =
+  for i in 0..<c.transCon.forStmt.len - 2:
+    if c.transCon.forStmt[i].kind == nkVarTuple:
+      for j in 0..<c.transCon.forStmt[i].len-1:
+        let lhs = c.transCon.forStmt[i][j]
+        let rhs = transform(c, newTupleAccess(c.graph, newTupleAccess(c.graph, e, i), j))
+        result.add(asgnTo(lhs, rhs))
+    else:
+      let lhs = c.transCon.forStmt[i]
+      let rhs = transform(c, newTupleAccess(c.graph, e, i))
+      result.add(asgnTo(lhs, rhs))
+
+
 proc transformYield(c: PTransf, n: PNode): PNode =
   proc asgnTo(lhs: PNode, rhs: PNode): PNode =
     # Choose the right assignment instruction according to the given ``lhs``
@@ -400,7 +413,8 @@ proc transformYield(c: PTransf, n: PNode): PNode =
           let lhs = c.transCon.forStmt[i]
           let rhs = transform(c, v)
           result.add(asgnTo(lhs, rhs))
-    elif e.kind notin {nkAddr, nkHiddenAddr}: # no need to generate temp for address operation
+    elif e.kind notin {nkAddr, nkHiddenAddr} and e.kind != nkSym:
+      # no need to generate temp for address operation + nodes without sideeffects
       # TODO do not use temp for nodes which cannot have side-effects
       var tmp = newTemp(c, e.typ, e.info)
       let v = newNodeI(nkVarSection, e.info)
@@ -408,21 +422,9 @@ proc transformYield(c: PTransf, n: PNode): PNode =
 
       result.add transform(c, v)
 
-      for i in 0..<c.transCon.forStmt.len - 2:
-        if c.transCon.forStmt[i].kind == nkVarTuple:
-          for j in 0..<c.transCon.forStmt[i].len-1:
-            let lhs = c.transCon.forStmt[i][j]
-            let rhs = transform(c, newTupleAccess(c.graph, newTupleAccess(c.graph, tmp, i), j))
-            result.add(asgnTo(lhs, rhs))
-        else:
-          let lhs = c.transCon.forStmt[i]
-          let rhs = transform(c, newTupleAccess(c.graph, tmp, i))
-          result.add(asgnTo(lhs, rhs))
+      assignTupleUnpacking(c, tmp)
     else:
-      for i in 0..<c.transCon.forStmt.len - 2:
-        let lhs = c.transCon.forStmt[i]
-        let rhs = transform(c, newTupleAccess(c.graph, e, i))
-        result.add(asgnTo(lhs, rhs))
+      assignTupleUnpacking(c, e)
   else:
     if c.transCon.forStmt[0].kind == nkVarTuple:
       var notLiteralTuple = false # we don't generate temp for tuples with const value: (1, 2, 3)
@@ -435,7 +437,8 @@ proc transformYield(c: PTransf, n: PNode): PNode =
       else:
         notLiteralTuple = true
 
-      if e.kind notin {nkAddr, nkHiddenAddr} and notLiteralTuple:
+      if e.kind notin {nkAddr, nkHiddenAddr} and notLiteralTuple and e.kind != nkSym:
+        # no need to generate temp for address operation + nodes without sideeffects
         # TODO do not use temp for nodes which cannot have side-effects
         var tmp = newTemp(c, e.typ, e.info)
         let v = newNodeI(nkVarSection, e.info)
diff --git a/tests/arc/t24402.nim b/tests/arc/t24402.nim
new file mode 100644
index 0000000000..7de7489d80
--- /dev/null
+++ b/tests/arc/t24402.nim
@@ -0,0 +1,54 @@
+discard """
+  joinable: false
+"""
+
+# bug #24402
+
+iterator myPairsInline*[T](twoDarray: seq[seq[T]]): (int, seq[T]) {.inline.} =
+  for indexValuePair in twoDarray.pairs:
+    yield indexValuePair
+
+iterator myPairsClosure*[T](twoDarray: seq[seq[T]]): (int, seq[T]) {.closure.} =
+  for indexValuePair in twoDarray.pairs:
+    yield indexValuePair
+
+template testTotalMem(iter: untyped): int =
+  proc innerTestTotalMem(): int {.gensym.} =
+    result = 0
+
+    # do the same operation 100 times, which should have similar mem footprint
+    # as doing it once.
+    for iterNum in 0..100:
+      result = max(result, getTotalMem()) # record current mem footprint
+
+      # initialize nested sequence
+      var my2dArray: seq[seq[int32]] = @[]
+
+      # fill with some data...
+      for i in 0'i32..10_000:
+        var z = @[i, i+1]
+        my2dArray.add z
+
+      # use that data somehow...
+      var otherContainer: seq[int32] = @[]
+      var count = 0'i32
+      for oneDindex, innerArray in my2dArray.iter:
+        for value in innerArray:
+          inc count
+          if oneDindex > 50 and value < 200:
+            otherContainer.add count
+
+  innerTestTotalMem()
+
+proc main =
+  let closureMem = testTotalMem(myPairsClosure) #1052672
+  let inlineMem = testTotalMem(myPairsInline) #20328448
+
+  when defined(echoFootprint):
+    echo "Closure memory footprint: " & $closureMem
+    echo "Inline memory footprint: " & $inlineMem
+
+  # check that mem footprint is relatively similar b/t each method
+  doAssert (closureMem - inlineMem).abs < (closureMem div 10)
+
+main()
\ No newline at end of file