From 0933fb7f142e1c3c86b68c13680daf6a81761bc6 Mon Sep 17 00:00:00 2001 From: Andrii Riabushenko <cdome@bk.ru> Date: Wed, 8 Jan 2020 10:11:17 +0000 Subject: [PATCH 1/7] make sink operator optional --- compiler/injectdestructors.nim | 78 +++++++++++++--------------------- compiler/liftdestructors.nim | 30 ++++++++----- doc/destructors.rst | 8 +++- tests/destructor/smart_ptr.nim | 6 --- 4 files changed, 54 insertions(+), 68 deletions(-) diff --git a/compiler/injectdestructors.nim b/compiler/injectdestructors.nim index 84473aabc9190..d1fac23ac6e14 100644 --- a/compiler/injectdestructors.nim +++ b/compiler/injectdestructors.nim @@ -230,6 +230,10 @@ proc genOp(c: Con; t: PType; kind: TTypeAttachedOp; dest, ri: PNode): PNode = addrExp.add(dest) result = newTree(nkCall, newSymNode(op), addrExp) +proc genDestroy(c: Con; dest: PNode): PNode = + let t = dest.typ.skipTypes({tyGenericInst, tyAlias, tySink}) + result = genOp(c, t, attachedDestructor, dest, nil) + when false: proc preventMoveRef(dest, ri: PNode): bool = let lhs = dest.typ.skipTypes({tyGenericInst, tyAlias, tySink}) @@ -248,22 +252,22 @@ proc canBeMoved(c: Con; t: PType): bool {.inline.} = proc genSink(c: var Con; dest, ri: PNode): PNode = if isFirstWrite(dest, c): # optimize sink call into a bitwise memcopy - result = newTree(nkFastAsgn, dest) + result = newTree(nkFastAsgn, dest, ri) else: let t = dest.typ.skipTypes({tyGenericInst, tyAlias, tySink}) - let k = if t.attachedOps[attachedSink] != nil: attachedSink - else: attachedAsgn - if t.attachedOps[k] != nil: - result = genOp(c, t, k, dest, ri) + if t.attachedOps[attachedSink] != nil: + result = genOp(c, t, attachedSink, dest, ri) + result.add ri else: - # in rare cases only =destroy exists but no sink or assignment - # (see Pony object in tmove_objconstr.nim) - # we generate a fast assignment in this case: - result = newTree(nkFastAsgn, dest) + # the default is to use combination of `=destroy(dest)` and + # and copyMem(dest, source). This is efficient. + let snk = newTree(nkFastAsgn, dest, ri) + result = newTree(nkStmtList, genDestroy(c, dest), snk) proc genCopyNoCheck(c: Con; dest, ri: PNode): PNode = let t = dest.typ.skipTypes({tyGenericInst, tyAlias, tySink}) result = genOp(c, t, attachedAsgn, dest, ri) + result.add ri proc genCopy(c: var Con; dest, ri: PNode): PNode = let t = dest.typ @@ -273,10 +277,6 @@ proc genCopy(c: var Con; dest, ri: PNode): PNode = checkForErrorPragma(c, t, ri, "=") result = genCopyNoCheck(c, dest, ri) -proc genDestroy(c: Con; dest: PNode): PNode = - let t = dest.typ.skipTypes({tyGenericInst, tyAlias, tySink}) - result = genOp(c, t, attachedDestructor, dest, nil) - proc addTopVar(c: var Con; v: PNode) = c.topLevelVars.add newTree(nkIdentDefs, v, c.emptyNode, c.emptyNode) @@ -298,8 +298,6 @@ proc genDefaultCall(t: PType; c: Con; info: TLineInfo): PNode = proc destructiveMoveVar(n: PNode; c: var Con): PNode = # generate: (let tmp = v; reset(v); tmp) - # XXX: Strictly speaking we can only move if there is a ``=sink`` defined - # or if no ``=sink`` is defined and also no assignment. if not hasDestructor(n.typ): result = copyTree(n) else: @@ -344,9 +342,7 @@ proc passCopyToSink(n: PNode; c: var Con): PNode = # out of loops we need to mark it as 'wasMoved'. result.add genWasMoved(tmp, c) if hasDestructor(n.typ): - var m = genCopy(c, tmp, n) - m.add p(n, c, normal) - result.add m + result.add genCopy(c, tmp, p(n, c, normal)) if isLValue(n) and not isClosureEnv(n): message(c.graph.config, n.info, hintPerformance, ("passing '$1' to a sink parameter introduces an implicit copy; " & @@ -438,9 +434,7 @@ proc ensureDestruction(arg: PNode; c: var Con): PNode = # This was already done in the sink parameter handling logic. result = newNodeIT(nkStmtListExpr, arg.info, arg.typ) let tmp = getTemp(c, arg.typ, arg.info) - var sinkExpr = genSink(c, tmp, arg) - sinkExpr.add arg - result.add sinkExpr + result.add genSink(c, tmp, arg) result.add tmp c.destroys.add genDestroy(c, tmp) else: @@ -596,8 +590,7 @@ proc p(n: PNode; c: var Con; mode: ProcessMode): PNode = if ri.kind == nkEmpty and c.inLoop > 0: ri = genDefaultCall(v.typ, c, v.info) if ri.kind != nkEmpty: - let r = moveOrCopy(v, ri, c) - result.add r + result.add moveOrCopy(v, ri, c) else: # keep the var but transform 'ri': var v = copyNode(n) var itCopy = copyNode(it) @@ -629,8 +622,7 @@ proc p(n: PNode; c: var Con; mode: ProcessMode): PNode = result.add call else: let tmp = getTemp(c, n[0].typ, n.info) - var m = genCopyNoCheck(c, tmp, n[0]) - m.add p(n[0], c, normal) + let m = genCopyNoCheck(c, tmp, p(n[0], c, normal)) result = newTree(nkStmtList, genWasMoved(tmp, c), m) var toDisarm = n[0] if toDisarm.kind == nkStmtListExpr: toDisarm = toDisarm.lastSon @@ -665,8 +657,7 @@ proc moveOrCopy(dest, ri: PNode; c: var Con): PNode = if isUnpackedTuple(dest): result = newTree(nkFastAsgn, dest, p(ri, c, consumed)) else: - result = genSink(c, dest, ri) - result.add p(ri, c, consumed) + result = genSink(c, dest, p(ri, c, consumed)) of nkBracketExpr: if isUnpackedTuple(ri[0]): # unpacking of tuple: take over elements @@ -674,37 +665,30 @@ proc moveOrCopy(dest, ri: PNode; c: var Con): PNode = elif isAnalysableFieldAccess(ri, c.owner) and isLastRead(ri, c): # Rule 3: `=sink`(x, z); wasMoved(z) var snk = genSink(c, dest, ri) - snk.add ri result = newTree(nkStmtList, snk, genWasMoved(ri, c)) else: - result = genCopy(c, dest, ri) - result.add p(ri, c, consumed) + result = genCopy(c, dest, p(ri, c, consumed)) of nkBracket: # array constructor if ri.len > 0 and isDangerousSeq(ri.typ): - result = genCopy(c, dest, ri) + result = genCopy(c, dest, p(ri, c, consumed)) else: - result = genSink(c, dest, ri) - result.add p(ri, c, consumed) + result = genSink(c, dest, p(ri, c, consumed)) of nkObjConstr, nkTupleConstr, nkClosure, nkCharLit..nkNilLit: - result = genSink(c, dest, ri) - result.add p(ri, c, consumed) + result = genSink(c, dest, p(ri, c, consumed)) of nkSym: if isSinkParam(ri.sym): # Rule 3: `=sink`(x, z); wasMoved(z) sinkParamIsLastReadCheck(c, ri) - var snk = genSink(c, dest, ri) - snk.add ri + let snk = genSink(c, dest, ri) result = newTree(nkStmtList, snk, genWasMoved(ri, c)) elif ri.sym.kind != skParam and ri.sym.owner == c.owner and isLastRead(ri, c) and canBeMoved(c, dest.typ): # Rule 3: `=sink`(x, z); wasMoved(z) - var snk = genSink(c, dest, ri) - snk.add ri + let snk = genSink(c, dest, ri) result = newTree(nkStmtList, snk, genWasMoved(ri, c)) else: - result = genCopy(c, dest, ri) - result.add p(ri, c, consumed) + result = genCopy(c, dest, p(ri, c, consumed)) of nkHiddenSubConv, nkHiddenStdConv, nkConv: when false: result = moveOrCopy(dest, ri[1], c) @@ -713,8 +697,7 @@ proc moveOrCopy(dest, ri: PNode; c: var Con): PNode = copyRi[1] = result[^1] result[^1] = copyRi else: - result = genSink(c, dest, ri) - result.add p(ri, c, sinkArg) + result = genSink(c, dest, p(ri, c, sinkArg)) of nkObjDownConv, nkObjUpConv: when false: result = moveOrCopy(dest, ri[0], c) @@ -722,20 +705,17 @@ proc moveOrCopy(dest, ri: PNode; c: var Con): PNode = copyRi[0] = result[^1] result[^1] = copyRi else: - result = genSink(c, dest, ri) - result.add p(ri, c, sinkArg) + result = genSink(c, dest, p(ri, c, sinkArg)) of nkStmtListExpr, nkBlockExpr, nkIfExpr, nkCaseStmt: handleNested(ri): moveOrCopy(dest, node, c) else: if isAnalysableFieldAccess(ri, c.owner) and isLastRead(ri, c) and canBeMoved(c, dest.typ): # Rule 3: `=sink`(x, z); wasMoved(z) - var snk = genSink(c, dest, ri) - snk.add ri + let snk = genSink(c, dest, ri) result = newTree(nkStmtList, snk, genWasMoved(ri, c)) else: - result = genCopy(c, dest, ri) - result.add p(ri, c, consumed) + result = genCopy(c, dest, p(ri, c, consumed)) proc computeUninit(c: var Con) = if not c.uninitComputed: diff --git a/compiler/liftdestructors.nim b/compiler/liftdestructors.nim index daf91954b05be..4d3ddd746cd30 100644 --- a/compiler/liftdestructors.nim +++ b/compiler/liftdestructors.nim @@ -720,18 +720,26 @@ proc produceSym(g: ModuleGraph; c: PContext; typ: PType; kind: TTypeAttachedOp; # register this operation already: typ.attachedOps[kind] = result - var tk: TTypeKind - if g.config.selectedGC in {gcArc, gcOrc, gcHooks}: - tk = skipTypes(typ, {tyOrdinal, tyRange, tyInferred, tyGenericInst, tyStatic, tyAlias, tySink}).kind - else: - tk = tyNone # no special casing for strings and seqs - case tk - of tySequence: - fillSeqOp(a, typ, body, d, newSymNode(src)) - of tyString: - fillStrOp(a, typ, body, d, newSymNode(src)) + + if kind == attachedSink and typ.attachedOps[attachedDestructor] != nil and + sfOverriden in typ.attachedOps[attachedDestructor].flags: + ## compiler can use a combination of `=destroy` and memCopy for sink op + dest.flags.incl sfCursor + body.add newOpCall(typ.attachedOps[attachedDestructor], d) + body.add newAsgnStmt(d, newSymNode(src)) else: - fillBody(a, typ, body, d, newSymNode(src)) + var tk: TTypeKind + if g.config.selectedGC in {gcArc, gcOrc, gcHooks}: + tk = skipTypes(typ, {tyOrdinal, tyRange, tyInferred, tyGenericInst, tyStatic, tyAlias, tySink}).kind + else: + tk = tyNone # no special casing for strings and seqs + case tk + of tySequence: + fillSeqOp(a, typ, body, d, newSymNode(src)) + of tyString: + fillStrOp(a, typ, body, d, newSymNode(src)) + else: + fillBody(a, typ, body, d, newSymNode(src)) var n = newNodeI(nkProcDef, info, bodyPos+1) for i in 0..<n.len: n[i] = newNodeI(nkEmpty, info) diff --git a/doc/destructors.rst b/doc/destructors.rst index 5877b53c38607..db6dbef900dcb 100644 --- a/doc/destructors.rst +++ b/doc/destructors.rst @@ -53,7 +53,8 @@ written as: a.data[i] = b.data[i] proc `=sink`*[T](a: var myseq[T]; b: myseq[T]) = - # move assignment + # move assignment, optional. + # Compiler is using `=destroy` and `copyMem` when not provided `=destroy`(a) a.len = b.len a.cap = b.cap @@ -130,7 +131,10 @@ A `=sink` hook moves an object around, the resources are stolen from the source and passed to the destination. It is ensured that source's destructor does not free the resources afterwards by setting the object to its default value (the value the object's state started in). Setting an object ``x`` back to its -default value is written as ``wasMoved(x)``. +default value is written as ``wasMoved(x)``. When not provided compiler +is using a combination of `=destroy` and `copyMem` instead. This is a efficient +hence users rarely need to implement their own `=sink` operator, it is enough to +provide `=destroy` and `=`, compiler will take care about the rest. The prototype of this hook for a type ``T`` needs to be: diff --git a/tests/destructor/smart_ptr.nim b/tests/destructor/smart_ptr.nim index 7c3141d226907..5079dc9dbc88d 100644 --- a/tests/destructor/smart_ptr.nim +++ b/tests/destructor/smart_ptr.nim @@ -20,12 +20,6 @@ proc `=`*[T](dest: var SharedPtr[T], src: SharedPtr[T]) {.inline.} = discard atomicInc(src.val[].atomicCounter) dest.val = src.val -proc `=sink`*[T](dest: var SharedPtr[T], src: SharedPtr[T]) {.inline.} = - if dest.val != src.val: - if dest.val != nil: - `=destroy`(dest) - dest.val = src.val - proc newSharedPtr*[T](val: sink T): SharedPtr[T] = result.val = cast[type(result.val)](allocShared0(sizeof(result.val[]))) result.val.atomicCounter = 1 From eac309ca74d5f38a3d91bc504be3dd8455fb51ee Mon Sep 17 00:00:00 2001 From: Andrii Riabushenko <cdome@bk.ru> Date: Wed, 8 Jan 2020 22:07:51 +0000 Subject: [PATCH 2/7] bug fix, add changelog entry --- changelog.md | 4 ++++ compiler/liftdestructors.nim | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/changelog.md b/changelog.md index b3e5745c6da36..3eec76c247f68 100644 --- a/changelog.md +++ b/changelog.md @@ -67,6 +67,10 @@ - An `align` pragma can now be used for variables and object fields, similar to the `alignas` declaration modifier in C/C++. +- `=sink` type bound operator is now optional. Compiler can now use combination + of `=destroy` and `copyMem` to move objects efficiently. + + ## Language changes - Unsigned integer operators have been fixed to allow promotion of the first operand. diff --git a/compiler/liftdestructors.nim b/compiler/liftdestructors.nim index 4d3ddd746cd30..4897fb768a5bc 100644 --- a/compiler/liftdestructors.nim +++ b/compiler/liftdestructors.nim @@ -725,7 +725,7 @@ proc produceSym(g: ModuleGraph; c: PContext; typ: PType; kind: TTypeAttachedOp; sfOverriden in typ.attachedOps[attachedDestructor].flags: ## compiler can use a combination of `=destroy` and memCopy for sink op dest.flags.incl sfCursor - body.add newOpCall(typ.attachedOps[attachedDestructor], d) + body.add newOpCall(typ.attachedOps[attachedDestructor], d[0]) body.add newAsgnStmt(d, newSymNode(src)) else: var tk: TTypeKind From 12816b9f30d9b911020e6c21bcbfeb3e4e88b211 Mon Sep 17 00:00:00 2001 From: Andrii Riabushenko <cdome@bk.ru> Date: Wed, 8 Jan 2020 22:40:44 +0000 Subject: [PATCH 3/7] Trigger build From 812d31cb313a40cddbc8490a87f97fb8eff48e3e Mon Sep 17 00:00:00 2001 From: Andrii Riabushenko <cdome@bk.ru> Date: Tue, 14 Jan 2020 09:36:55 +0000 Subject: [PATCH 4/7] fix one regression --- compiler/injectdestructors.nim | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/compiler/injectdestructors.nim b/compiler/injectdestructors.nim index d1fac23ac6e14..d70b672ca3e0c 100644 --- a/compiler/injectdestructors.nim +++ b/compiler/injectdestructors.nim @@ -267,7 +267,6 @@ proc genSink(c: var Con; dest, ri: PNode): PNode = proc genCopyNoCheck(c: Con; dest, ri: PNode): PNode = let t = dest.typ.skipTypes({tyGenericInst, tyAlias, tySink}) result = genOp(c, t, attachedAsgn, dest, ri) - result.add ri proc genCopy(c: var Con; dest, ri: PNode): PNode = let t = dest.typ @@ -342,7 +341,9 @@ proc passCopyToSink(n: PNode; c: var Con): PNode = # out of loops we need to mark it as 'wasMoved'. result.add genWasMoved(tmp, c) if hasDestructor(n.typ): - result.add genCopy(c, tmp, p(n, c, normal)) + var m = genCopy(c, tmp, n) + m.add p(n, c, normal) + result.add m if isLValue(n) and not isClosureEnv(n): message(c.graph.config, n.info, hintPerformance, ("passing '$1' to a sink parameter introduces an implicit copy; " & @@ -622,7 +623,8 @@ proc p(n: PNode; c: var Con; mode: ProcessMode): PNode = result.add call else: let tmp = getTemp(c, n[0].typ, n.info) - let m = genCopyNoCheck(c, tmp, p(n[0], c, normal)) + var m = genCopyNoCheck(c, tmp, n[0]) + m.add p(n[0], c, normal) result = newTree(nkStmtList, genWasMoved(tmp, c), m) var toDisarm = n[0] if toDisarm.kind == nkStmtListExpr: toDisarm = toDisarm.lastSon @@ -667,11 +669,13 @@ proc moveOrCopy(dest, ri: PNode; c: var Con): PNode = var snk = genSink(c, dest, ri) result = newTree(nkStmtList, snk, genWasMoved(ri, c)) else: - result = genCopy(c, dest, p(ri, c, consumed)) + result = genCopy(c, dest, ri) + result.add p(ri, c, consumed) of nkBracket: # array constructor if ri.len > 0 and isDangerousSeq(ri.typ): - result = genCopy(c, dest, p(ri, c, consumed)) + result = genCopy(c, dest, ri) + result.add p(ri, c, consumed) else: result = genSink(c, dest, p(ri, c, consumed)) of nkObjConstr, nkTupleConstr, nkClosure, nkCharLit..nkNilLit: @@ -688,7 +692,8 @@ proc moveOrCopy(dest, ri: PNode; c: var Con): PNode = let snk = genSink(c, dest, ri) result = newTree(nkStmtList, snk, genWasMoved(ri, c)) else: - result = genCopy(c, dest, p(ri, c, consumed)) + result = genCopy(c, dest, ri) + result.add p(ri, c, consumed) of nkHiddenSubConv, nkHiddenStdConv, nkConv: when false: result = moveOrCopy(dest, ri[1], c) @@ -715,7 +720,8 @@ proc moveOrCopy(dest, ri: PNode; c: var Con): PNode = let snk = genSink(c, dest, ri) result = newTree(nkStmtList, snk, genWasMoved(ri, c)) else: - result = genCopy(c, dest, p(ri, c, consumed)) + result = genCopy(c, dest, ri) + result.add p(ri, c, consumed) proc computeUninit(c: var Con) = if not c.uninitComputed: From bd12296c55a3ebe7afb74d0187ed3feb19ecc92c Mon Sep 17 00:00:00 2001 From: Andrii Riabushenko <cdome@bk.ru> Date: Wed, 15 Jan 2020 22:18:48 +0000 Subject: [PATCH 5/7] fix test --- tests/destructor/tdestructor.nim | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/tests/destructor/tdestructor.nim b/tests/destructor/tdestructor.nim index 780a45288755a..5cfecea4ef9ea 100644 --- a/tests/destructor/tdestructor.nim +++ b/tests/destructor/tdestructor.nim @@ -20,14 +20,10 @@ mygeneric2 destroyed ---- ---- myobj destroyed -myobj destroyed -myobj destroyed -myobj destroyed mygeneric1 destroyed --- myobj destroyed myobj destroyed -myobj destroyed ''' """ @@ -37,8 +33,10 @@ type p: pointer proc `=destroy`(o: var TMyObj) = - if o.p != nil: dealloc o.p - echo "myobj destroyed" + if o.p != nil: + dealloc o.p + o.p = nil + echo "myobj destroyed" type TMyGeneric1[T] = object From 53ce63bb0a5035dd027e83ca9355c795c3a95dc5 Mon Sep 17 00:00:00 2001 From: Andrii Riabushenko <cdome@bk.ru> Date: Thu, 16 Jan 2020 08:00:01 +0000 Subject: [PATCH 6/7] Trigger build From 58fc04105c485d2d6c6c607e66876d36f3b91be9 Mon Sep 17 00:00:00 2001 From: Andrii Riabushenko <cdome@bk.ru> Date: Thu, 16 Jan 2020 23:07:19 +0000 Subject: [PATCH 7/7] fix typos --- doc/destructors.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/destructors.rst b/doc/destructors.rst index 41010de1b6303..d2027102a17c8 100644 --- a/doc/destructors.rst +++ b/doc/destructors.rst @@ -131,8 +131,8 @@ A `=sink` hook moves an object around, the resources are stolen from the source and passed to the destination. It is ensured that source's destructor does not free the resources afterwards by setting the object to its default value (the value the object's state started in). Setting an object ``x`` back to its -default value is written as ``wasMoved(x)``. When not provided compiler -is using a combination of `=destroy` and `copyMem` instead. This is a efficient +default value is written as ``wasMoved(x)``. When not provided the compiler +is using a combination of `=destroy` and `copyMem` instead. This is efficient hence users rarely need to implement their own `=sink` operator, it is enough to provide `=destroy` and `=`, compiler will take care about the rest.