Skip to content

Commit

Permalink
fixes #24402; Memory leak under Arc/Orc on inline iterators with nest…
Browse files Browse the repository at this point in the history
…ed 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.
  • Loading branch information
ringabout authored Nov 12, 2024
1 parent 1863f64 commit 21420d8
Show file tree
Hide file tree
Showing 2 changed files with 73 additions and 16 deletions.
35 changes: 19 additions & 16 deletions compiler/transf.nim
Original file line number Diff line number Diff line change
Expand Up @@ -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``
Expand Down Expand Up @@ -400,29 +413,18 @@ 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)
v.addVar(tmp, e)

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)
Expand All @@ -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)
Expand Down
54 changes: 54 additions & 0 deletions tests/arc/t24402.nim
Original file line number Diff line number Diff line change
@@ -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()

0 comments on commit 21420d8

Please sign in to comment.