search

nim-lang / Nim

Nim is a statically typed compiled systems programming language. It combines successful concepts from mature languages like Python, Ada and Modula. Its design focuses on efficiency, expressiveness, and elegance (in that order of priority).
https://nim-lang.org
Other
16.62k stars 1.47k forks source link

The hook `=destroy` not called if cycled ref exists when using `orc` #24161

Open forchid opened 2 months ago

forchid commented 2 months ago

Description

The test source

import std/strformat

type
    Person = object of RootObj
        name: string
        age: uint
    Student = object of Person
        id: uint
        self: StudentRef
    StudentRef = ref Student

proc say(s: Student) =
    echo(fmt"Student {s.name}#{s.id} and {s.age} years old")

proc `=destroy`(s: var Student) = # Here `var` compatible with nim-1.6.20
    echo(fmt"Student {s.name}#{s.id} destroyed.")

proc test(useCycle: bool, id: uint) =
    echo(fmt"Use cycled ref? {useCycle}")
    var t = StudentRef(name: "James", age: 36, id: id)
    say(t[])
    # `=destroy` not called if cycled ref exists when using orc!
    if useCycle: 
        t.self = t

test(false, 1)
echo("========================================")
test(true,  2)

Nim Version

The nim 1.6.20

>nim -v
Nim Compiler Version 1.6.20 [Windows: amd64]
Compiled at 2024-04-07
Copyright (c) 2006-2023 by Andreas Rumpf

active boot switches: -d:release

The nim 2.0.8

>%nim20_home%\bin\nim -v
Nim Compiler Version 2.0.8 [Windows: amd64]
Compiled at 2024-07-03
Copyright (c) 2006-2023 by Andreas Rumpf

active boot switches: -d:release

Current Output

# The nim 1.6.20

nim\mem>nim c -d:release --mm:orc dnc.nim
Hint: used config file 'nim-1.6.20\config\nim.cfg' [Conf]
Hint: used config file 'nim-1.6.20\config\config.nims' [Conf]
..........................................................................
nim\mem\dnc.nim(15, 6) template/generic instantiation from here
nim\mem\dnc.nim(16, 13) Warning: formatValue(fmtRes_486539341, s.name, "") can raise an unlisted exception: ref ValueError [Effect]
Hint:  [Link]
Hint: gc: orc; opt: speed; options: -d:release
42215 lines; 0.800s; 48.746MiB peakmem; proj: nim\mem\dnc.nim; out: nim\mem\dnc.exe [SuccessX]

nim\mem>dnc
Use cycled ref? false
Student James#1 and 36 years old
Student James#1 destroyed.
========================================
Use cycled ref? true
Student James#2 and 36 years old

# The nim 2.0.8
```shell
nim\mem>%nim20_home%\bin\nim c -d:release --mm:orc dnc.nim
Hint: used config file 'nim-2.0.8\config\nim.cfg' [Conf]
Hint: used config file 'nim-2.0.8\config\config.nims' [Conf]
...........................................................................................
nim\mem\dnc.nim(15, 1) Warning: A custom '=destroy' hook which takes a 'var T' parameter is deprecated; it should take a 'T' parameter [Deprecated]
CC: dnc.nim
Hint:  [Link]
Hint: mm: orc; threads: on; opt: speed; options: -d:release
42545 lines; 0.910s; 57.543MiB peakmem; proj: nim\mem\dnc.nim; out: nim\mem\dnc.exe [SuccessX]

nim\mem>dnc
Use cycled ref? false
Student James#1 and 36 years old
Student James#1 destroyed.
========================================
Use cycled ref? true
Student James#2 and 36 years old

### Expected Output

```text
# The nim 1.6.20

nim\mem>dnc
Use cycled ref? false
Student James#1 and 36 years old
Student James#1 destroyed.
========================================
Use cycled ref? true
Student James#2 and 36 years old
Student James#2 destroyed.

# The nim 2.0.8
```shell
nim\mem>dnc
Use cycled ref? false
Student James#1 and 36 years old
Student James#1 destroyed.
========================================
Use cycled ref? true
Student James#2 and 36 years old
Student James#2 destroyed.


### Known Workarounds

_No response_

### Additional Information

_No response_
Araq commented 2 months ago

Run it in a loop and see that it does get collected:


import std/strformat

type
    Person = object of RootObj
        name: string
        age: uint
    Student = object of Person
        id: uint
        self: StudentRef
    StudentRef = ref Student

proc say(s: Student) =
    echo(fmt"Student {s.name}#{s.id} and {s.age} years old")

proc `=destroy`(s: var Student) = # Here `var` compatible with nim-1.6.20
    echo(fmt"Student {s.name}#{s.id} destroyed.")

proc test(useCycle: bool, id: uint) =
    echo(fmt"Use cycled ref? {useCycle}")
    var t = StudentRef(name: "James", age: 36, id: id)
    say(t[])
    # `=destroy` not called if cycled ref exists when using orc!
    if useCycle:
        t.self = t

for _ in 0..1000:
  test(true,  2)

And no, you really don't want the cycle collector to run on every scope exit, the performance would be ridiculously bad. (Been there, done that.)

forchid commented 2 months ago

Run it in a loop and see that it does get collected:

import std/strformat

type
    Person = object of RootObj
        name: string
        age: uint
    Student = object of Person
        id: uint
        self: StudentRef
    StudentRef = ref Student

proc say(s: Student) =
    echo(fmt"Student {s.name}#{s.id} and {s.age} years old")

proc `=destroy`(s: var Student) = # Here `var` compatible with nim-1.6.20
    echo(fmt"Student {s.name}#{s.id} destroyed.")

proc test(useCycle: bool, id: uint) =
    echo(fmt"Use cycled ref? {useCycle}")
    var t = StudentRef(name: "James", age: 36, id: id)
    say(t[])
    # `=destroy` not called if cycled ref exists when using orc!
    if useCycle:
        t.self = t

for _ in 0..1000:
  test(true,  2)

And no, you really don't want the cycle collector to run on every scope exit, the performance would be ridiculously bad. (Been there, done that.)

@Araq But the orc collector looks not hard realtime. As this issue shown, these collections of recyled ref objects are delayed.

Araq commented 2 months ago

I never claimed it to be hard realtime. Hard realtime requires you to either use arc/atomicArc or to disable the cycle collector and schedule it yourself when appropriate much like the older refc does.

forchid commented 1 month ago

@Araq out of memory happened if ref the other object in cycled ref relation and return the referenced object in the proc test.

The test source

import std/strutils

type
    Person = object of RootObj
        name: string
        age: uint
        data: array[1 shl 24, uint8]
    Student = object of Person
        id: uint
        self: StudentRef
    StudentRef = ref Student

proc say(s: Student) =
    #echo(fmt"Student {s.name}#{s.id} and {s.age} years old")
    discard

proc `=destroy`(s: var Student) = # Here `var` compatible with nim-1.6.20
    #echo(fmt"Student {s.name}#{s.id} destroyed.")
    discard

proc test(useCycle: bool, id: uint): StudentRef =
    #echo(fmt"Use cycled ref? {useCycle}")
    var t = StudentRef(name: "James", age: 36, id: id)
    say(t[])
    # `=destroy` not called if cycled ref exists when using orc!
    if useCycle:
        var oth = StudentRef(name: "Tome", age: 37, id: id + 1)
        oth.self = t
        t.self = oth # Ref the other ref
    return t   

for i in 0..100000:
  var t = test(true,  2)
  t.self = nil
  if (i + 1) mod 1000 == 0: echo "Occupied mem " & $formatSize(getOccupiedMem())

The test result

`out of memory` if using `--mm:orc`!
mem>nim c -d:release --mm:orc dnc_araq.nim
Hint: used config file 'nim-1.6.20\config\nim.cfg' [Conf]
Hint: used config file 'nim-1.6.20\config\config.nims' [Conf]
.........................................................................
CC: dnc_araq.nim
Hint:  [Link]
Hint: gc: orc; opt: speed; options: -d:release
38201 lines; 0.907s; 48.793MiB peakmem; proj: mem\dnc_araq.nim; out: mem\dnc_araq.exe [SuccessX]

mem>dnc_araq
Occupied mem 16.113GiB
out of memory

But if using the --mm:refc, it works.

mem>nim c -d:release --mm:refc dnc_araq.nim
Hint: used config file 'nim-1.6.20\config\nim.cfg' [Conf]
Hint: used config file 'nim-1.6.20\config\config.nims' [Conf]
..........................................................................
CC: ../../../../Programs/nim-1.6.20/lib/std/private/digitsutils.nim
CC: ../../../../Programs/nim-1.6.20/lib/system/dollars.nim
CC: ../../../../Programs/nim-1.6.20/lib/system/io.nim
CC: ../../../../Programs/nim-1.6.20/lib/system.nim
CC: ../../../../Programs/nim-1.6.20/lib/pure/strutils.nim
CC: dnc_araq.nim
Hint:  [Link]
Hint: gc: refc; opt: speed; options: -d:release
39362 lines; 4.783s; 48.777MiB peakmem; proj: mem\dnc_araq.nim; out: mem\dnc_araq.exe [SuccessX]

mem>dnc_araq
Occupied mem 82.559MiB
Occupied mem 49.559MiB
Occupied mem 49.559MiB
Occupied mem 49.559MiB
Occupied mem 49.559MiB
Occupied mem 49.559MiB
forchid commented 1 month ago

@Araq This case is the size of per object 4k.

The test source

import std/strutils

type
    Person = object of RootObj
        name: string
        age: uint
        data: array[1 shl 12, uint8]
    Student = object of Person
        id: uint
        self: StudentRef
    StudentRef = ref Student

proc say(s: Student) =
    #echo(fmt"Student {s.name}#{s.id} and {s.age} years old")
    discard

proc `=destroy`(s: var Student) = # Here `var` compatible with nim-1.6.20
    #echo(fmt"Student {s.name}#{s.id} destroyed.")
    discard

proc test(useCycle: bool, id: uint): StudentRef =
    #echo(fmt"Use cycled ref? {useCycle}")
    var t = StudentRef(name: "James", age: 36, id: id)
    say(t[])
    # `=destroy` not called if cycled ref exists when using orc!
    if useCycle:
        var oth = StudentRef(name: "Tome", age: 37, id: id + 1)
        oth.self = t
        t.self = oth # Ref the other ref
        #t.self = t
    return t   

var i = 0
while true:
  var t = test(true,  2)
  t.self = nil
  i.inc
  if i mod 10_0000 == 0: 
    echo $i & ": Occupied mem " & $formatSize(getOccupiedMem())

The test result

Using orc

mem>dnc_araq
100000: Occupied mem 781.656MiB
200000: Occupied mem 1.526GiB
300000: Occupied mem 2.29GiB
400000: Occupied mem 3.052GiB
500000: Occupied mem 3.817GiB
600000: Occupied mem 4.578GiB
700000: Occupied mem 5.343GiB
800000: Occupied mem 6.108GiB
900000: Occupied mem 6.867GiB
1000000: Occupied mem 7.632GiB
1100000: Occupied mem 8.397GiB
1200000: Occupied mem 9.162GiB
1300000: Occupied mem 9.919GiB
1400000: Occupied mem 10.683GiB
1500000: Occupied mem 11.449GiB
1600000: Occupied mem 12.214GiB
1700000: Occupied mem 12.977GiB
1800000: Occupied mem 13.743GiB
1900000: Occupied mem 14.506GiB
2000000: Occupied mem 15.26GiB
2100000: Occupied mem 16.025GiB
2200000: Occupied mem 16.789GiB
2300000: Occupied mem 17.554GiB
out of memory

Using refc

mem>dnc_araq
100000: Occupied mem 3.988MiB
200000: Occupied mem 3.988MiB
300000: Occupied mem 3.988MiB
400000: Occupied mem 3.988MiB
500000: Occupied mem 3.988MiB
600000: Occupied mem 3.988MiB
700000: Occupied mem 3.988MiB
800000: Occupied mem 3.988MiB
900000: Occupied mem 3.988MiB
1000000: Occupied mem 3.988MiB
1100000: Occupied mem 3.988MiB
1200000: Occupied mem 3.988MiB
1300000: Occupied mem 3.988MiB
1400000: Occupied mem 3.988MiB
1500000: Occupied mem 3.988MiB
1600000: Occupied mem 3.988MiB
1700000: Occupied mem 3.988MiB
1800000: Occupied mem 3.988MiB
1900000: Occupied mem 3.988MiB
2000000: Occupied mem 3.988MiB
2100000: Occupied mem 3.988MiB
2200000: Occupied mem 3.988MiB
2300000: Occupied mem 3.988MiB
...
11000000: Occupied mem 3.988MiB
11100000: Occupied mem 3.988MiB
11200000: Occupied mem 3.988MiB
11300000: Occupied mem 3.988MiB
...
forchid commented 1 month ago

The case of the object size 256 bytes.

mem>dnc_araq
100000: Occupied mem 30.924MiB
200000: Occupied mem 61.645MiB
300000: Occupied mem 92.459MiB
400000: Occupied mem 122.477MiB
500000: Occupied mem 154.65MiB
600000: Occupied mem 183.715MiB
700000: Occupied mem 215.686MiB
800000: Occupied mem 248.766MiB
900000: Occupied mem 275.564MiB
1000000: Occupied mem 308.238MiB
1100000: Occupied mem 340.318MiB
1200000: Occupied mem 373.086MiB
1300000: Occupied mem 397.135MiB
1400000: Occupied mem 429.309MiB
1500000: Occupied mem 462.389MiB
1600000: Occupied mem 495.156MiB
1700000: Occupied mem 525.674MiB
...
6300000: Occupied mem 1.912GiB
6400000: Occupied mem 1.942GiB
6500000: Occupied mem 1.938GiB
6600000: Occupied mem 1.97GiB
6700000: Occupied mem 2.001GiB
...
33100000: Occupied mem 9.871GiB
33200000: Occupied mem 9.905GiB
33300000: Occupied mem 9.934GiB
33400000: Occupied mem 9.969GiB
33500000: Occupied mem 9.999GiB
33600000: Occupied mem 10.029GiB
...
57200000: Occupied mem 17.219GiB
57300000: Occupied mem 17.249GiB
57400000: Occupied mem 17.278GiB
57500000: Occupied mem 17.308GiB
57600000: Occupied mem 17.338GiB
out of memory
Araq commented 1 month ago

Looks like the collection heuristics need some tweaking :P

ringabout commented 1 month ago

eh, don't you need to destroy the memory of self: StudentRef?

proc `=destroy`(s: var Student) = # Here `var` compatible with nim-1.6.20
    #echo(fmt"Student {s.name}#{s.id} destroyed.")
    discard

Like

proc `=destroy`(s: Student)  = # Here `var` compatible with nim-1.6.20
    # echo("Student  destroyed.")
    `=destroy`(s.name)
    `=destroy`(s.self)
100000: Occupied mem 28.016KiB
200000: Occupied mem 28.016KiB
300000: Occupied mem 28.016KiB
400000: Occupied mem 28.016KiB
500000: Occupied mem 28.016KiB
600000: Occupied mem 28.016KiB
700000: Occupied mem 28.016KiB
800000: Occupied mem 28.016KiB
900000: Occupied mem 28.016KiB
1000000: Occupied mem 28.016KiB
1100000: Occupied mem 28.016KiB
1200000: Occupied mem 28.016KiB
1300000: Occupied mem 28.016KiB
1400000: Occupied mem 28.016Ki
forchid commented 1 month ago

eh, don't you need to destroy the memory of self: StudentRef?

@ringabout No need. The ref object is traced by nim gc collector. If manually destroying the field self in =destroy, this breaks the cycled ref relation manually and recyle the self(acutally this object maybe need to keep alive for other object referencing it! )

Araq commented 1 month ago

You do need to destroy the name field though it should make no difference for your test program.

forchid commented 1 month ago

@Araq I find that memory leak also exists in the case of no cycled ref and only referenced when using orc or arc. This is where the problem.

The test source

import std/strutils

type
    Person = object of RootObj
        name: string
        age: uint
        data: array[1 shl 8, uint8]
    Student = object of Person
        id: uint
        self: StudentRef
    StudentRef = ref Student

proc say(s: Student) =
    #echo(fmt"Student {s.name}#{s.id} and {s.age} years old")
    discard

proc `=destroy`(s: var Student) = # Here `var` compatible with nim-1.6.20
    #echo(fmt"Student {s.name}#{s.id} destroyed.")
    `=destroy`(s.name)
    discard

proc test(refed: bool, id: uint): StudentRef =
    var t = StudentRef(name: "James", age: 36, id: id)
    say(t[])
    # `=destroy` not called if cycled ref exists when using orc!
    if refed:
        var oth = StudentRef(name: "Tome", age: 37, id: id + 1)
        oth.self = t # No cycled ref relation, only referenced!
    return t   

var i = 0
while true:
  var t = test(true,  2)
  i.inc
  if i mod 10_0000 == 0: 
    echo $i & ": Occupied mem " & $formatSize(getOccupiedMem())

The test result

mem>dnc_araq_nc
100000: Occupied mem 30.924MiB
200000: Occupied mem 61.645MiB
300000: Occupied mem 92.459MiB
400000: Occupied mem 122.477MiB
500000: Occupied mem 154.65MiB
...
20200000: Occupied mem 6.135GiB
20300000: Occupied mem 6.165GiB
20400000: Occupied mem 6.195GiB
20500000: Occupied mem 6.225GiB
20600000: Occupied mem 6.255GiB
...
forchid commented 1 month ago

@Araq If seting the field self as nil(this no problem instead of calling =destroy) in the hook =destroy, this test passes. So the nim bug is that the gc collector orc can't recyle the other object B after recyling A in the cycled ref relation such as A <=> B and B returned from a proc. And the refc no this problem.

The test source passed

import std/strutils

type
    Person = object of RootObj
        name: string
        age: uint
        data: array[1 shl 8, uint8]
    Student = object of Person
        id: uint
        self: StudentRef
    StudentRef = ref Student

proc say(s: Student) =
    #echo(fmt"Student {s.name}#{s.id} and {s.age} years old")
    discard

proc `=destroy`(s: var Student) = # Here `var` compatible with nim-1.6.20
    #echo(fmt"Student {s.name}#{s.id} destroyed.")
    s.self = nil # ok
    discard

proc test(useCycle: bool, id: uint): StudentRef =
    #echo(fmt"Use cycled ref? {useCycle}")
    var t = StudentRef(name: "James", age: 36, id: id)
    say(t[])
    # `=destroy` not called if cycled ref exists when using orc!
    if useCycle:
        var oth = StudentRef(name: "Tome", age: 37, id: id + 1)
        oth.self = t
        t.self = oth # Ref the other ref
    return t   

var i = 0
while true:
  var t = test(true,  2)
  t.self = nil
  i.inc
  if i mod 10_0000 == 0: 
    echo $i & ": Occupied mem " & $formatSize(getOccupiedMem())

The test result passed

mem>dnc_araq
100000: Occupied mem 20.328KiB
200000: Occupied mem 20.328KiB
300000: Occupied mem 20.328KiB
400000: Occupied mem 20.328KiB
500000: Occupied mem 20.328KiB
600000: Occupied mem 20.328KiB
700000: Occupied mem 20.328KiB
800000: Occupied mem 20.328KiB
900000: Occupied mem 20.328KiB
1000000: Occupied mem 20.328KiB
1100000: Occupied mem 20.328KiB
1200000: Occupied mem 20.328KiB
1300000: Occupied mem 20.328KiB
1400000: Occupied mem 20.328KiB
1500000: Occupied mem 20.328KiB
1600000: Occupied mem 20.328KiB
1700000: Occupied mem 20.328KiB
1800000: Occupied mem 20.328KiB
1900000: Occupied mem 20.328KiB
2000000: Occupied mem 20.328KiB
2100000: Occupied mem 20.328KiB
2200000: Occupied mem 20.328KiB
2300000: Occupied mem 20.328KiB
2400000: Occupied mem 20.328KiB
...
forchid commented 1 month ago

@Araq If using the default =destroy instead of overriding it, no problem. And a programmer must set the ref field as nil manually if overriding the hook destroy. The collector refc no memory leak for it using default destroy instead of this overrided =destroy. So I'll close this issue.

The test source

import std/strutils

type
    Person = object of RootObj
        name: string
        age: uint
        data: array[1 shl 8, uint8]
    Student = object of Person
        id: uint
        self: StudentRef
    StudentRef = ref Student

proc say(s: Student) =
    discard

# The collector `refc` no memory leak for using default destroy instead of this `=destroy`
#proc `=destroy`(s: var Student) = # Here `var` compatible with nim-1.6.20
    #s.self = nil # Must set this field as nil if overriding `=destroy` otherwise memory leak!
    #discard

proc test(useCycle: bool, id: uint): StudentRef =
    var t = StudentRef(name: "James", age: 36, id: id)
    say(t[])
    if useCycle:
        var oth = StudentRef(name: "Tome", age: 37, id: id + 1)
        oth.self = t
        t.self = oth # Reference the other ref
    return t   

var i = 0
while true:
  var t = test(true,  2)
  t.self = nil # break cyclic ref for `arc`
  i.inc
  if i mod 10_0000 == 0: 
    echo $i & ": Occupied mem " & $formatSize(getOccupiedMem())
forchid commented 1 month ago

Using the default hook destroy, or set the ref field as nil if overriding =destroy otherwise memory leak!

Araq commented 1 month ago

This needs test cases and documentation as it's not obvious. :-) Previously the interactions between overridden destructors and the cycle collector were undefined.