From f71f9f83c2de79d57e89a7b6c3d66225f42c1482 Mon Sep 17 00:00:00 2001 From: Araq Date: Sun, 14 Jan 2018 17:34:27 +0100 Subject: [PATCH] GC improvements; distinguish between thread local and globals in the marking step --- compiler/ccgstmts.nim | 8 ++- lib/system/alloc.nim | 8 +-- lib/system/gc.nim | 117 ++++++++++++++++----------------------- lib/system/gc2.nim | 24 +++----- lib/system/gc_common.nim | 24 ++++++++ lib/system/gc_ms.nim | 23 ++------ tests/gc/gctest.nim | 52 ++++++++++------- tests/gc/gctest.nim.cfg | 1 + 8 files changed, 129 insertions(+), 128 deletions(-) create mode 100644 tests/gc/gctest.nim.cfg diff --git a/compiler/ccgstmts.nim b/compiler/ccgstmts.nim index 36816cc2c0..c9642b74d3 100644 --- a/compiler/ccgstmts.nim +++ b/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), - "#nimRegisterGlobalMarker($1);$n", [prc]) + if sfThread in v.flags: + 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 diff --git a/lib/system/alloc.nim b/lib/system/alloc.nim index e274e8e0cd..46f75396b2 100644 --- a/lib/system/alloc.nim +++ b/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 = diff --git a/lib/system/gc.nim b/lib/system/gc.nim index a0c943c350..cd5c6870dc 100644 --- a/lib/system/gc.nim +++ b/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 - additionalRoots: CellSeq # dummy roots for GC_ref/unref + marked: CellSet + 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.additionalRoots) + init(gch.marked) + init(gch.additionalRoots) when hasThreadSupport: init(gch.toDispose) - -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 + gch.isMainThread = true 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(isAllocatedPtr(gch.region, cell), "forAllChildren: 2") - gcAssert(cell.typ != nil, "forAllChildren: 3") - gcAssert cell.typ.kind in {tyRef, tyOptAsRef, tySequence, tyString}, "forAllChildren: 4" + gcAssert(cell != nil, "forAllChildren: cell is nil") + gcAssert(isAllocatedPtr(gch.region, cell), "forAllChildren: pointer not part of the heap") + gcAssert(cell.typ != nil, "forAllChildren: cell.typ is nil") + 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) = - for x in allObjects(gch.region): - if isCell(x): - # cast to PCell is correct here: - var c = cast[PCell](x) - if c notin gch.marked: freeCyclicCell(gch, c) +proc sweep(gch: var GcHeap) = + for x in allObjects(gch.region): + if isCell(x): + # cast to PCell is correct here: + var c = cast[PCell](x) + if c notin gch.marked: freeCyclicCell(gch, c) -when useMarkForDebug or useBackupGc: - proc markS(gch: var GcHeap, c: PCell) = - incl(gch.marked, c) - gcAssert gch.tempStack.len == 0, "stack not empty!" - forAllChildren(c, waMarkPrecise) - while gch.tempStack.len > 0: - dec gch.tempStack.len - var d = gch.tempStack.d[gch.tempStack.len] - gcAssert isAllocatedPtr(gch.region, d), "markS: foreign heap root detected!" - if not containsOrIncl(gch.marked, d): - forAllChildren(d, waMarkPrecise) +proc markS(gch: var GcHeap, c: PCell) = + gcAssert isAllocatedPtr(gch.region, c), "markS: foreign heap root detected A!" + incl(gch.marked, c) + gcAssert gch.tempStack.len == 0, "stack not empty!" + forAllChildren(c, waMarkPrecise) + while gch.tempStack.len > 0: + dec gch.tempStack.len + var d = gch.tempStack.d[gch.tempStack.len] + gcAssert isAllocatedPtr(gch.region, d), "markS: foreign heap root detected B!" + if not containsOrIncl(gch.marked, d): + 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 .. gch.additionalRoots.len-1: markS(gch, d[i]) + for i in 0 .. threadLocalMarkersLen-1: threadLocalMarkers[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: - # could point to a cell which we don't own and don't want to touch/trace - if isAllocatedPtr(gch.region, c): - markS(gch, c) - else: + when hasThreadSupport: + # 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): 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) - var d = gch.decStack.d - for i in 0..gch.decStack.len-1: - sysAssert isAllocatedPtr(gch.region, d[i]), "collectCycles" - markS(gch, d[i]) - markGlobals(gch) - sweep(gch) + cellsetReset(gch.marked) + var d = gch.decStack.d + for i in 0..gch.decStack.len-1: + sysAssert isAllocatedPtr(gch.region, d[i]), "collectCycles" + markS(gch, d[i]) + markGlobals(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) diff --git a/lib/system/gc2.nim b/lib/system/gc2.nim index d57a01dc75..cd90c6d622 100644 --- a/lib/system/gc2.nim +++ b/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 diff --git a/lib/system/gc_common.nim b/lib/system/gc_common.nim index 484a4db9a0..f0abd918a2 100644 --- a/lib/system/gc_common.nim +++ b/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 diff --git a/lib/system/gc_ms.nim b/lib/system/gc_ms.nim index 5fc48d848b..0754f2cfee 100644 --- a/lib/system/gc_ms.nim +++ b/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]) diff --git a/tests/gc/gctest.nim b/tests/gc/gctest.nim index f5c81f033a..6d7cf9985d 100644 --- a/tests/gc/gctest.nim +++ b/tests/gc/gctest.nim @@ -179,24 +179,38 @@ proc main() = write(stdout, "done!\n") var - father: TBNode - s: 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)) + father {.threadvar.}: TBNode + s {.threadvar.}: string -write(stdout, "starting main...\n") -main() + fatherAsGlobal: TBNode -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") +proc start = + 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") + 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() diff --git a/tests/gc/gctest.nim.cfg b/tests/gc/gctest.nim.cfg new file mode 100644 index 0000000000..aed303eef8 --- /dev/null +++ b/tests/gc/gctest.nim.cfg @@ -0,0 +1 @@ +--threads:on