GC improvements; distinguish between thread local and globals in the marking step

compiler/ccgstmts.nim

@@ -21,8 +21,12 @@ proc registerGcRoot(p: BProc, v: PSym) =
     # we register a specialized marked proc here; this has the advantage
     # that it works out of the box for thread local storage then :-)
     let prc = genTraverseProcForGlobal(p.module, v, v.info)
-    appcg(p.module, p.module.initProc.procSec(cpsInit),
+    if sfThread in v.flags:
-      "#nimRegisterGlobalMarker($1);$n", [prc])
+      appcg(p.module, p.module.initProc.procSec(cpsInit),
+        "#nimRegisterThreadLocalMarker($1);$n", [prc])
+    else:
+      appcg(p.module, p.module.initProc.procSec(cpsInit),
+        "#nimRegisterGlobalMarker($1);$n", [prc])
 
 proc isAssignedImmediately(n: PNode): bool {.inline.} =
   if n.kind == nkEmpty: return false

lib/system/alloc.nim

@@ -813,13 +813,13 @@ proc alloc0(allocator: var MemRegion, size: Natural): pointer =
   zeroMem(result, size)
 
 proc dealloc(allocator: var MemRegion, p: pointer) =
-  sysAssert(p != nil, "dealloc 0")
+  sysAssert(p != nil, "dealloc: p is nil")
   var x = cast[pointer](cast[ByteAddress](p) -% sizeof(FreeCell))
-  sysAssert(x != nil, "dealloc 1")
+  sysAssert(x != nil, "dealloc: x is nil")
   sysAssert(isAccessible(allocator, x), "is not accessible")
-  sysAssert(cast[ptr FreeCell](x).zeroField == 1, "dealloc 2")
+  sysAssert(cast[ptr FreeCell](x).zeroField == 1, "dealloc: object header corrupted")
   rawDealloc(allocator, x)
-  sysAssert(not isAllocatedPtr(allocator, x), "dealloc 3")
+  sysAssert(not isAllocatedPtr(allocator, x), "dealloc: object still accessible")
   track("dealloc", p, 0)
 
 proc realloc(allocator: var MemRegion, p: pointer, newsize: Natural): pointer =

lib/system/gc.nim

@@ -21,10 +21,6 @@ const
                       # reaches this threshold
                       # this seems to be a good value
   withRealTime = defined(useRealtimeGC)
-  useMarkForDebug = defined(gcGenerational)
-  useBackupGc = true                      # use a simple M&S GC to collect
-                                          # cycles instead of the complex
-                                          # algorithm
 
 when withRealTime and not declared(getTicks):
   include "system/timers"
@@ -92,14 +88,12 @@ type
       maxPause: Nanos        # max allowed pause in nanoseconds; active if > 0
     region: MemRegion        # garbage collected region
     stat: GcStat
-    when useMarkForDebug or useBackupGc:
+    marked: CellSet
-      marked: CellSet
+    additionalRoots: CellSeq # dummy roots for GC_ref/unref
-      additionalRoots: CellSeq # dummy roots for GC_ref/unref
     when hasThreadSupport:
       toDispose: SharedList[pointer]
+    isMainThread: bool
 
-{.deprecated: [TWalkOp: WalkOp, TFinalizer: Finalizer, TGcHeap: GcHeap,
-              TGcStat: GcStat].}
 var
   gch {.rtlThreadVar.}: GcHeap
 
@@ -314,27 +308,11 @@ proc initGC() =
     init(gch.zct)
     init(gch.tempStack)
     init(gch.decStack)
-    when useMarkForDebug or useBackupGc:
+    init(gch.marked)
-      init(gch.marked)
+    init(gch.additionalRoots)
-      init(gch.additionalRoots)
     when hasThreadSupport:
       init(gch.toDispose)
-
+    gch.isMainThread = true
-when useMarkForDebug or useBackupGc:
-  type
-    GlobalMarkerProc = proc () {.nimcall, benign.}
-  {.deprecated: [TGlobalMarkerProc: GlobalMarkerProc].}
-  var
-    globalMarkersLen: int
-    globalMarkers: array[0.. 7_000, GlobalMarkerProc]
-
-  proc nimRegisterGlobalMarker(markerProc: GlobalMarkerProc) {.compilerProc.} =
-    if globalMarkersLen <= high(globalMarkers):
-      globalMarkers[globalMarkersLen] = markerProc
-      inc globalMarkersLen
-    else:
-      echo "[GC] cannot register global variable; too many global variables"
-      quit 1
 
 proc cellsetReset(s: var CellSet) =
   deinit(s)
@@ -377,10 +355,10 @@ proc forAllChildrenAux(dest: pointer, mt: PNimType, op: WalkOp) =
     else: discard
 
 proc forAllChildren(cell: PCell, op: WalkOp) =
-  gcAssert(cell != nil, "forAllChildren: 1")
+  gcAssert(cell != nil, "forAllChildren: cell is nil")
-  gcAssert(isAllocatedPtr(gch.region, cell), "forAllChildren: 2")
+  gcAssert(isAllocatedPtr(gch.region, cell), "forAllChildren: pointer not part of the heap")
-  gcAssert(cell.typ != nil, "forAllChildren: 3")
+  gcAssert(cell.typ != nil, "forAllChildren: cell.typ is nil")
-  gcAssert cell.typ.kind in {tyRef, tyOptAsRef, tySequence, tyString}, "forAllChildren: 4"
+  gcAssert cell.typ.kind in {tyRef, tyOptAsRef, tySequence, tyString}, "forAllChildren: unknown GC'ed type"
   let marker = cell.typ.marker
   if marker != nil:
     marker(cellToUsr(cell), op.int)
@@ -623,30 +601,31 @@ proc freeCyclicCell(gch: var GcHeap, c: PCell) =
     gcAssert(c.typ != nil, "freeCyclicCell")
     zeroMem(c, sizeof(Cell))
 
-when useBackupGc:
+proc sweep(gch: var GcHeap) =
-  proc sweep(gch: var GcHeap) =
+  for x in allObjects(gch.region):
-    for x in allObjects(gch.region):
+    if isCell(x):
-      if isCell(x):
+      # cast to PCell is correct here:
-        # cast to PCell is correct here:
+      var c = cast[PCell](x)
-        var c = cast[PCell](x)
+      if c notin gch.marked: freeCyclicCell(gch, c)
-        if c notin gch.marked: freeCyclicCell(gch, c)
 
-when useMarkForDebug or useBackupGc:
+proc markS(gch: var GcHeap, c: PCell) =
-  proc markS(gch: var GcHeap, c: PCell) =
+  gcAssert isAllocatedPtr(gch.region, c), "markS: foreign heap root detected A!"
-    incl(gch.marked, c)
+  incl(gch.marked, c)
-    gcAssert gch.tempStack.len == 0, "stack not empty!"
+  gcAssert gch.tempStack.len == 0, "stack not empty!"
-    forAllChildren(c, waMarkPrecise)
+  forAllChildren(c, waMarkPrecise)
-    while gch.tempStack.len > 0:
+  while gch.tempStack.len > 0:
-      dec gch.tempStack.len
+    dec gch.tempStack.len
-      var d = gch.tempStack.d[gch.tempStack.len]
+    var d = gch.tempStack.d[gch.tempStack.len]
-      gcAssert isAllocatedPtr(gch.region, d), "markS: foreign heap root detected!"
+    gcAssert isAllocatedPtr(gch.region, d), "markS: foreign heap root detected B!"
-      if not containsOrIncl(gch.marked, d):
+    if not containsOrIncl(gch.marked, d):
-        forAllChildren(d, waMarkPrecise)
+      forAllChildren(d, waMarkPrecise)
 
-  proc markGlobals(gch: var GcHeap) =
+proc markGlobals(gch: var GcHeap) =
+  if gch.isMainThread:
     for i in 0 .. globalMarkersLen-1: globalMarkers[i]()
-    let d = gch.additionalRoots.d
+  for i in 0 .. threadLocalMarkersLen-1: threadLocalMarkers[i]()
-    for i in 0 .. gch.additionalRoots.len-1: markS(gch, d[i])
+  let d = gch.additionalRoots.d
+  for i in 0 .. gch.additionalRoots.len-1: markS(gch, d[i])
 
 when logGC:
   var
@@ -690,16 +669,15 @@ proc doOperation(p: pointer, op: WalkOp) =
   of waPush:
     add(gch.tempStack, c)
   of waMarkGlobal:
-    when useMarkForDebug or useBackupGc:
+    when hasThreadSupport:
-      when hasThreadSupport:
+      # could point to a cell which we don't own and don't want to touch/trace
-        # could point to a cell which we don't own and don't want to touch/trace
+      # XXX: This should not be required anymore!
-        if isAllocatedPtr(gch.region, c):
+      if isAllocatedPtr(gch.region, c):
-          markS(gch, c)
-      else:
         markS(gch, c)
+    else:
+      markS(gch, c)
   of waMarkPrecise:
-    when useMarkForDebug or useBackupGc:
+    add(gch.tempStack, c)
-      add(gch.tempStack, c)
   #of waDebug: debugGraph(c)
 
 proc nimGCvisit(d: pointer, op: int) {.compilerRtl.} =
@@ -713,14 +691,13 @@ proc collectCycles(gch: var GcHeap) =
       nimGCunref(c)
   # ensure the ZCT 'color' is not used:
   while gch.zct.len > 0: discard collectZCT(gch)
-  when useBackupGc:
+  cellsetReset(gch.marked)
-    cellsetReset(gch.marked)
+  var d = gch.decStack.d
-    var d = gch.decStack.d
+  for i in 0..gch.decStack.len-1:
-    for i in 0..gch.decStack.len-1:
+    sysAssert isAllocatedPtr(gch.region, d[i]), "collectCycles"
-      sysAssert isAllocatedPtr(gch.region, d[i]), "collectCycles"
+    markS(gch, d[i])
-      markS(gch, d[i])
+  markGlobals(gch)
-    markGlobals(gch)
+  sweep(gch)
-    sweep(gch)
 
 proc gcMark(gch: var GcHeap, p: pointer) {.inline.} =
   # the addresses are not as cells on the stack, so turn them to cells:
@@ -861,7 +838,7 @@ proc collectCT(gch: var GcHeap) =
   if (gch.zct.len >= stackMarkCosts or (cycleGC and
       getOccupiedMem(gch.region)>=gch.cycleThreshold) or alwaysGC) and
       gch.recGcLock == 0:
-    when useMarkForDebug:
+    when false:
       prepareForInteriorPointerChecking(gch.region)
       cellsetReset(gch.marked)
       markForDebug(gch)

lib/system/gc2.nim

@@ -104,6 +104,7 @@ type
     pDumpHeapFile: pointer # File that is used for GC_dumpHeap
     when hasThreadSupport:
       toDispose: SharedList[pointer]
+    isMainThread: bool
 
 var
   gch {.rtlThreadVar.}: GcHeap
@@ -134,6 +135,7 @@ proc initGC() =
     init(gch.greyStack)
     when hasThreadSupport:
       init(gch.toDispose)
+    gch.isMainThread = true
 
 # Which color to use for new objects is tricky: When we're marking,
 # they have to be *white* so that everything is marked that is only
@@ -284,20 +286,6 @@ proc unsureAsgnRef(dest: PPointer, src: pointer) {.compilerProc.} =
     if not isOnStack(dest): markGrey(s)
   dest[] = src
 
-type
-  GlobalMarkerProc = proc () {.nimcall, benign.}
-var
-  globalMarkersLen: int
-  globalMarkers: array[0.. 7_000, GlobalMarkerProc]
-
-proc nimRegisterGlobalMarker(markerProc: GlobalMarkerProc) {.compilerProc.} =
-  if globalMarkersLen <= high(globalMarkers):
-    globalMarkers[globalMarkersLen] = markerProc
-    inc globalMarkersLen
-  else:
-    echo "[GC] cannot register global variable; too many global variables"
-    quit 1
-
 proc forAllSlotsAux(dest: pointer, n: ptr TNimNode, op: WalkOp) {.benign.} =
   var d = cast[ByteAddress](dest)
   case n.kind
@@ -492,7 +480,9 @@ proc GC_dumpHeap*(file: File) =
         c_fprintf(file, "onstack %p\n", d[i])
       else:
         c_fprintf(file, "onstack_invalid %p\n", d[i])
-  for i in 0 .. globalMarkersLen-1: globalMarkers[i]()
+  if gch.isMainThread:
+    for i in 0 .. globalMarkersLen-1: globalMarkers[i]()
+  for i in 0 .. threadLocalMarkersLen-1: threadLocalMarkers[i]()
   while true:
     let x = allObjectsAsProc(gch.region, addr spaceIter)
     if spaceIter.state < 0: break
@@ -579,7 +569,9 @@ proc markIncremental(gch: var GcHeap): bool =
   result = true
 
 proc markGlobals(gch: var GcHeap) =
-  for i in 0 .. globalMarkersLen-1: globalMarkers[i]()
+  if gch.isMainThread:
+    for i in 0 .. globalMarkersLen-1: globalMarkers[i]()
+  for i in 0 .. threadLocalMarkersLen-1: threadLocalMarkers[i]()
 
 proc doOperation(p: pointer, op: WalkOp) =
   if p == nil: return

lib/system/gc_common.nim

@@ -393,3 +393,27 @@ proc deallocHeap*(runFinalizers = true; allowGcAfterwards = true) =
   zeroMem(addr gch.region, sizeof(gch.region))
   if allowGcAfterwards:
     initGC()
+
+type
+  GlobalMarkerProc = proc () {.nimcall, benign.}
+var
+  globalMarkersLen: int
+  globalMarkers: array[0.. 3499, GlobalMarkerProc]
+  threadLocalMarkersLen: int
+  threadLocalMarkers: array[0.. 3499, GlobalMarkerProc]
+
+proc nimRegisterGlobalMarker(markerProc: GlobalMarkerProc) {.compilerProc.} =
+  if globalMarkersLen <= high(globalMarkers):
+    globalMarkers[globalMarkersLen] = markerProc
+    inc globalMarkersLen
+  else:
+    echo "[GC] cannot register global variable; too many global variables"
+    quit 1
+
+proc nimRegisterThreadLocalMarker(markerProc: GlobalMarkerProc) {.compilerProc.} =
+  if threadLocalMarkersLen <= high(threadLocalMarkers):
+    threadLocalMarkers[threadLocalMarkersLen] = markerProc
+    inc threadLocalMarkersLen
+  else:
+    echo "[GC] cannot register thread local variable; too many thread local variables"
+    quit 1

lib/system/gc_ms.nim

@@ -40,8 +40,6 @@ type
     # A ref type can have a finalizer that is called before the object's
     # storage is freed.
 
-  GlobalMarkerProc = proc () {.nimcall, benign.}
-
   GcStat = object
     collections: int         # number of performed full collections
     maxThreshold: int        # max threshold that has been set
@@ -75,9 +73,9 @@ type
     stat: GcStat
     when hasThreadSupport:
       toDispose: SharedList[pointer]
+    isMainThread: bool
     additionalRoots: CellSeq # dummy roots for GC_ref/unref
-{.deprecated: [TWalkOp: WalkOp, TFinalizer: Finalizer, TGcStat: GcStat,
+
-              TGlobalMarkerProc: GlobalMarkerProc, TGcHeap: GcHeap].}
 var
   gch {.rtlThreadVar.}: GcHeap
 
@@ -119,18 +117,6 @@ proc unsureAsgnRef(dest: PPointer, src: pointer) {.inline.} =
 proc internRefcount(p: pointer): int {.exportc: "getRefcount".} =
   result = 0
 
-var
-  globalMarkersLen: int
-  globalMarkers: array[0.. 7_000, GlobalMarkerProc]
-
-proc nimRegisterGlobalMarker(markerProc: GlobalMarkerProc) {.compilerProc.} =
-  if globalMarkersLen <= high(globalMarkers):
-    globalMarkers[globalMarkersLen] = markerProc
-    inc globalMarkersLen
-  else:
-    echo "[GC] cannot register global variable; too many global variables"
-    quit 1
-
 # this that has to equals zero, otherwise we have to round up UnitsPerPage:
 when BitsPerPage mod (sizeof(int)*8) != 0:
   {.error: "(BitsPerPage mod BitsPerUnit) should be zero!".}
@@ -234,6 +220,7 @@ proc initGC() =
       init(gch.marked)
     when hasThreadSupport:
       init(gch.toDispose)
+    gch.isMainThread = true
 
 proc forAllSlotsAux(dest: pointer, n: ptr TNimNode, op: WalkOp) {.benign.} =
   var d = cast[ByteAddress](dest)
@@ -450,7 +437,9 @@ when false:
           quit 1
 
 proc markGlobals(gch: var GcHeap) =
-  for i in 0 .. globalMarkersLen-1: globalMarkers[i]()
+  if gch.isMainThread:
+    for i in 0 .. globalMarkersLen-1: globalMarkers[i]()
+  for i in 0 .. threadLocalMarkersLen-1: threadLocalMarkers[i]()
   let d = gch.additionalRoots.d
   for i in 0 .. gch.additionalRoots.len-1: mark(gch, d[i])
 

tests/gc/gctest.nim

@@ -179,24 +179,38 @@ proc main() =
   write(stdout, "done!\n")
 
 var
-    father: TBNode
+  father {.threadvar.}: TBNode
-    s: string
+  s {.threadvar.}: string
-s = ""
-s = ""
-writeLine(stdout, repr(caseTree()))
-father.t.data = @["ha", "lets", "stress", "it"]
-father.t.data = @["ha", "lets", "stress", "it"]
-var t = buildTree()
-write(stdout, repr(t[]))
-buildBTree(father)
-write(stdout, repr(father))
 
-write(stdout, "starting main...\n")
+  fatherAsGlobal: TBNode
-main()
 
-GC_fullCollect()
+proc start =
-# the M&S GC fails with this call and it's unclear why. Definitely something
+  s = ""
-# we need to fix!
+  s = ""
-GC_fullCollect()
+  writeLine(stdout, repr(caseTree()))
-writeLine(stdout, GC_getStatistics())
+  father.t.data = @["ha", "lets", "stress", "it"]
-write(stdout, "finished\n")
+  father.t.data = @["ha", "lets", "stress", "it"]
+  var t = buildTree()
+  write(stdout, repr(t[]))
+  buildBTree(father)
+  write(stdout, repr(father))
+
+  write(stdout, "starting main...\n")
+  main()
+
+  GC_fullCollect()
+  # the M&S GC fails with this call and it's unclear why. Definitely something
+  # we need to fix!
+  #GC_fullCollect()
+  writeLine(stdout, GC_getStatistics())
+  write(stdout, "finished\n")
+
+#fatherAsGlobal.t.data = @["ha", "lets", "stress", "it"]
+#var tg = buildTree()
+#buildBTree(fatherAsGlobal)
+
+var thr: array[8, Thread[void]]
+for i in low(thr)..high(thr):
+  createThread(thr[i], start)
+joinThreads(thr)
+start()

tests/gc/gctest.nim.cfg

@@ -0,0 +1 @@
+--threads:on