Description
Although the "simple closures work" commit makes =destroy called for the elements of a closure environment on destruction of the closure with the "--gc:destructors" compiler switch, they aren't called generally for other Memory Management GC's (didn't check --newruntime).
This can cause memory leaks for "regular" GC's when fields of the environment contain objects containing fields that are manually allocated and depend on =destroy called on that object for their deallocation.
For the Alternative #2 using a big seq as a field, these are currently collected by GC's properly as shown by external resource management tools even though the occupied memory measured internally increases since seq is treated as a ref; however, this doesn't work with "--gc:destructors" even with the "simple closures work" patch as the occupied memory also continuously increases with each loop (confirmed externally) in spite of the destructor being called.
This issue will apply even more when "--seqsv2" becomes an option for all GC's as the new version of seq (and string) will depend on destructors, so will not work if =destroy is not called properly.
Example
Compiling with "--gc:destructors" now partially works (not for seq) as expected after the "simple closures work" PR, but using "regular" GC (of any type including default "refc") causes the problem behavior as the =destroy is not called on the environment contained TestObj.
import sugar
type
TestObj = object
big: pointer # Alternative 1
# big: seq[byte] # alternative 2
cntnts: int
proc `=destroy`(x: var TestObj) =
# to show when destroyed...
echo "destroying TestObj containing: ", x.cntnts
if x.big != nil: x.big.deallocShared # destruction for Alternative 1
# x.big.`=destroy` # destruction for Alternative 2 - not working for --gc:destructors!
x.wasMoved # system doesn't do this zeroing for us!
proc `=`(dst: var TestObj; src: TestObj) =
echo "copying TestObj containing from/to: ", src.cntnts, " ", dst.cntnts
dst.`=destroy`; dst.big = src.big; dst.cntnts = src.cntnts
proc `=sink`(dst: var TestObj; src: TestObj) =
echo "moving TestObj containing from/to: ", src.cntnts, " ", dst.cntnts
dst.`=destroy`; dst.big = src.big; dst.cntnts = src.cntnts
proc makeInnerClosure(): () -> void =
let c = TestObj(big: (1024*1024).allocShared0, cntnts: 42) # the TestObj to be captured
# let c = TestObj(big: newSeq[byte](1024*1024), cntnts: 42) #; c.big[] = newSeq[byte](1024*1024) # the TestObj to be captured
result = () => c.cntnts.echo # by this returned closure!
echo "as created: ", c.cntnts # doesn't force a copy as it's using DFA to eliminate it?
proc main() =
var tst: () -> void
tst.`=sink` makeInnerClosure()
stdout.write "as obtained: "; tst()
for _ in 1 .. 10:
GC_fullCollect() # in case GC hasn't yet actually disposed...
let strtmem = getOccupiedMem()
main()
GC_fullCollect() # in case GC hasn't yet actually disposed...
echo "change in occupied memory: ", (getOccupiedMem() - strtmem)Current Output
Repeated for every loop:
moving TestObj containing from/to: 42 0
destroying TestObj containing: 0
as created: 42
as obtained: 42
change in occupied memory: 1081272
Note: the change in occupied memory changes to a stable value after the first loop, and even at that is bigger than the expected 1024*1024 = 1048576 bytes it is expected to leak without proper destruction. The memory leak is confirmed by external resource monitoring tools, which when the number of loops is increased to 1000 show a continuous increase in memory use until the test program ends, when the memory use drops back to its original value.
Expected Output
Repeated for every loop:
moving TestObj containing from/to: 42 0
destroying TestObj containing: 0
as created: 42
as obtained: 42
destroying TestObj containing: 42
change in occupied memory: 0
The change in occupied memory should be 0. This output is as obtained from the latest devel including the "simple closures work" PR with the "--gc:destructors" compiler switch.
Possible Solution
Since the "simple closures work" PR patches make this work for "--gc:destructors" (where it didn't before that PR), the patch should not be limited to only when that compiler switch is on but on for all GC (and --newruntime?) compiler switches.
Additional Information
Tested on the devel branch as of 1-11-2019 including the "simple closures work" PR:
$ nim -v
Nim Compiler Version 1.1.0 [Windows: amd64]
Compiled at 2019-11-02
Copyright (c) 2006-2019 by Andreas Rumpf
active boot switches: -d:release