simplify and revert optional memory ordering (#36)

* simplify and revert optional memory ordering

* modernize ci

* slap the compiler aroun'

* run docs against mainline

.github/workflows/ci.yml

@@ -5,11 +5,14 @@ on:
 
   push:
     branches:
-      - master
+      - main
   pull_request:
     branches:
       - '*'
 
+concurrency:
+  group: ci-${{ github.ref }}
+  cancel-in-progress: true
 
 jobs:
   changes:
@@ -23,10 +26,10 @@ jobs:
       # For PRs the path filter check with Github API, so no need to checkout
       # for them.
       - if: github.event_name != 'pull_request'
-        name: Checkout (if not PR)
-        uses: actions/checkout@v2
+        name: Checkout
+        uses: actions/checkout@v4
 
-      - uses: dorny/paths-filter@v2
+      - uses: dorny/paths-filter@v3
         id: filter
         with:
           filters: |
@@ -51,41 +54,78 @@ jobs:
     strategy:
       fail-fast: false
       matrix:
-        os: ['macos-latest', 'ubuntu-latest']
-        nim: ['devel']
-    name: '${{ matrix.os }} (${{ matrix.nim }})'
+        os: [ubuntu-latest]
+        compiler:
+          - name: nim
+            version: version-2-0
+          - name: nimskull
+            version: "*"
+
+    name: '${{ matrix.os }} (${{ matrix.compiler.name }} ${{ matrix.compiler.version }})'
     runs-on: ${{ matrix.os }}
+
+    defaults:
+      run:
+        shell: bash
+        working-directory: project
+
     steps:
       - name: Checkout
         uses: actions/checkout@v4
         with:
-          path: ci
+          path: project
 
-      - name: Setup Nim
+      - name: Nim
+        if: matrix.compiler.name == 'nim'
         uses: alaviss/[email protected]
         with:
           path: nim
-          version: ${{ matrix.nim }}
+          version: ${{ matrix.compiler.version }}
+
+      - name: Nimskull
+        id: nimskull
+        if: matrix.compiler.name == 'nimskull'
+        uses: nim-works/[email protected]
+        with:
+          nimskull-version: ${{ matrix.compiler.version }}
+
+      - name: Fetch Nimble
+        if: matrix.compiler.name == 'nimskull'
+        uses: actions/[email protected]
+        with:
+          path: nimble
+          repository: alaviss/nimble
+          ref: nimskull
+
+      - name: Build Nimble
+        if: matrix.compiler.name == 'nimskull'
+        run: |
+          nim c -d:release -o:"$NIMSKULL_BIN/nimble" src/nimble.nim
+          # Add nimble binary folder to PATH
+          echo "$HOME/.nimble/bin" >> "$GITHUB_PATH"
+        working-directory: nimble
+        env:
+          NIMSKULL_BIN: ${{ steps.nimskull.outputs.bin-path }}
 
       - name: Dependencies
-        shell: bash
         run: |
-          cd ci
           nimble --accept install "https://github.com/nim-works/cps"
           nimble --accept install "https://github.com/disruptek/balls"
           nimble --accept develop
 
-      - name: Tests
-        shell: bash
+      - name: Tests (gcc)
         run: |
-          cd ci
-          balls --path="." --gc:arc --backend:c --define:danger --define:release
+          balls --path="." --cc:gcc --gc:arc --backend:c --define:danger --define:release
+
+      - name: Tests (clang)
+        run: |
+          balls --path="." --cc:clang --gc:arc --backend:c --define:danger --define:release
 
       - name: Build docs
-        if: ${{ matrix.docs }} == 'true'
-        shell: bash
+        if: >
+          github.event_name == 'push' && github.ref == 'refs/heads/main' &&
+          matrix.os == 'ubuntu-latest' && matrix.compiler.version == 'version-2-0'
         run: |
-          cd ci
           branch=${{ github.ref }}
           branch=${branch##*/}
           nimble doc --project --outdir:docs --path="." \
@@ -102,19 +142,23 @@ jobs:
           cp docs/{the,}index.html || true
       - name: Publish docs
         if: >
-          github.event_name == 'push' && github.ref == 'refs/heads/master' &&
-          matrix.os == 'ubuntu-latest' && matrix.nim == 'devel'
-        uses: crazy-max/ghaction-github-pages@v2.5.0
+          github.event_name == 'push' && github.ref == 'refs/heads/main' &&
+          matrix.os == 'ubuntu-latest' && matrix.compiler.version == 'version-2-0'
+        uses: crazy-max/ghaction-github-pages@v3.1.0
         with:
-          build_dir: ci/docs
+          build_dir: project/docs
         env:
           GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
 
   # Set check-required on this
   success:
     needs: build
+    if: always()
     runs-on: ubuntu-latest
     name: 'All check passes'
     steps:
-      - run: |
-          echo "This is a workaround for Github's broken software"
+      - if: contains(needs.*.result, 'failure') || contains(needs.*.result, 'cancelled')
+        name: 'Fail when previous jobs fails'
+        run: |
+          echo "::error::One of the previous jobs failed"
+          exit 1

loony.nim

@@ -57,20 +57,13 @@ proc toStrTuple*(tag: TagPtr): string =
   var res = (nptr:tag.nptr, idx:tag.idx)
   return $res
 
-proc fetchAddSlot(tag: TagPtr; w: uint): uint =
-  ## A convenience to fetchAdd the node's slot.
-  if tag == 0:
-    raise AssertionDefect.newException "tagptr was empty"
-  else:
-    result = fetchAddSlot(tag.node, idx tag, w)
-
-proc fetchTail(queue: LoonyQueue, moorder: MemoryOrder = moRelaxed): TagPtr =
+proc fetchTail(queue: LoonyQueue): TagPtr =
   ## get the TagPtr of the tail (nptr: NodePtr, idx: uint16)
-  TagPtr load(queue.tail, order = moorder)
+  TagPtr load(queue.tail, order = moRelaxed)
 
-proc fetchHead(queue: LoonyQueue, moorder: MemoryOrder = moRelaxed): TagPtr =
+proc fetchHead(queue: LoonyQueue): TagPtr =
   ## get the TagPtr of the head (nptr: NodePtr, idx: uint16)
-  TagPtr load(queue.head, order = moorder)
+  TagPtr load(queue.head, order = moRelaxed)
 
 proc fetchCurrTail(queue: LoonyQueue): NodePtr {.used.} =
   # get the NodePtr of the current tail
@@ -80,20 +73,20 @@ proc fetchCurrTail(queue: LoonyQueue): NodePtr {.used.} =
 # imported std/atomics or we export atomics.
 # For the sake of not polluting the users namespace I have changed these into procs.
 # Atomic inc of idx in (nptr: NodePtr, idx: uint16)
-proc fetchIncTail(queue: LoonyQueue, moorder: MemoryOrder = moAcquire): TagPtr =
-  cast[TagPtr](queue.tail.fetchAdd(1, order = moorder))
-proc fetchIncHead(queue: LoonyQueue, moorder: MemoryOrder = moAcquire): TagPtr =
-  cast[TagPtr](queue.head.fetchAdd(1, order = moorder))
+proc fetchIncTail(queue: LoonyQueue): TagPtr =
+  cast[TagPtr](queue.tail.fetchAdd(1, order = moAcquire))
+proc fetchIncHead(queue: LoonyQueue): TagPtr =
+  cast[TagPtr](queue.head.fetchAdd(1, order = moAcquire))
 
-proc compareAndSwapTail(queue: LoonyQueue, expect: var uint, swap: uint | TagPtr): bool =
-  queue.tail.compareExchange(expect, swap)
+proc compareAndSwapTail(queue: LoonyQueue; expect: var uint; swap: uint | TagPtr): bool =
+  queue.tail.compareExchange(expect, swap, moRelease, moRelaxed)
 
-proc compareAndSwapHead(queue: LoonyQueue, expect: var uint, swap: uint | TagPtr): bool =
-  queue.head.compareExchange(expect, swap)
+proc compareAndSwapHead(queue: LoonyQueue; expect: var uint; swap: uint | TagPtr): bool =
+  queue.head.compareExchange(expect, swap, moRelease, moRelaxed)
 
-proc compareAndSwapCurrTail(queue: LoonyQueue, expect: var uint,
+proc compareAndSwapCurrTail(queue: LoonyQueue; expect: var uint;
                             swap: uint | TagPtr): bool {.used.} =
-  queue.currTail.compareExchange(expect, swap)
+  queue.currTail.compareExchange(expect, swap, moRelease, moRelaxed)
 
 proc `=destroy`*[T](x: var LoonyQueueImpl[T]) =
   ## Destroy is completely operated on the basis that no other threads are
@@ -191,7 +184,7 @@ proc advTail[T](queue: LoonyQueue[T]; pel: uint; tag: TagPtr): AdvTail =
         result = AdvOnly
         break done
       # Get current tails next node
-      var next = origTail.fetchNext()
+      var next = fetchNext(origTail, moRelaxed)
       if cast[ptr Node](next).isNil():
         # Prepare the new node with our element in it
         var node = allocNode pel
@@ -242,7 +235,7 @@ proc advHead(queue: LoonyQueue; curr, h, t: var TagPtr): AdvHead =
       incrDeqCount h.node
       QueueEmpty
     else:
-      var next = fetchNext h.nptr
+      var next = fetchNext(h.nptr, moAcquire)
       # Equivalent to (nptr: NodePtr, idx: idx+=1)
       curr += 1
       block done:
@@ -297,12 +290,12 @@ proc pushImpl[T](queue: LoonyQueue[T], el: sink T,
     atomicThreadFence(ATOMIC_RELEASE)
   while true:
     # Enq proc begins with incr the index of node in TagPtr
-    var tag = fetchIncTail(queue)
+    var tag = queue.fetchIncTail()
     if likely(tag.idx < N):
       # FAST PATH OPERATION - 99% of push will enter here; we want the minimal
       # amount of necessary operations in this path.
       # Perform a FAA on our reserved slot which should be 0'd.
-      let prev = tag.fetchAddSlot pel
+      let prev = fetchAddSlot(tag.node, tag.idx, pel, moAcquire)
       case prev
       of 0, RESUME:
         break           # the slot was empty; we're good to go
@@ -353,17 +346,17 @@ proc isEmptyImpl(head, tail: TagPtr): bool =
 proc isEmpty*(queue: LoonyQueue): bool =
   ## This operation should only be used by internal code. The response for this
   ## operation is not precise.
-  let head = fetchHead queue
-  let tail = fetchTail queue
+  let head = queue.fetchHead()
+  let tail = queue.fetchTail()
   isEmptyImpl(head, tail)
 
 proc popImpl[T](queue: LoonyQueue[T]; forcedCoherence: static bool = false): T =
   assert not queue.isNil(), "The queue has not been initialised"
   while true:
     # Before incr the deq index, init check performed to determine if queue is empty.
     # Ensure head is loaded last to keep mem hot
-    var tail = fetchTail queue
-    var curr = fetchHead queue
+    var tail = queue.fetchTail()
+    var curr = queue.fetchHead()
     if isEmptyImpl(curr, tail):
       # Queue was empty; nil can be caught in cps w/ "while cont.running"
       when T is ref or T is ptr:
@@ -374,7 +367,7 @@ proc popImpl[T](queue: LoonyQueue[T]; forcedCoherence: static bool = false): T =
     var head = queue.fetchIncHead()
     if likely(head.idx < N):
       # FAST PATH OPS
-      var prev = head.fetchAddSlot READER
+      var prev = fetchAddSlot(head.node, head.idx, READER, moRelease)
       # Last slot in a node - init reclaim proc; if WRITER bit set then upper bits
       # contain a valid pointer to an enqd el that can be returned (see enqueue)
       if not unlikely((prev and SLOTMASK) == 0):

loony/node.nim

@@ -94,14 +94,11 @@ proc prepareElement*[T](el: var T): uint =
   result = cast[uint](el) or WRITER
   wasMoved el
 
-proc fetchNext*(node: var Node, moorder: MemoryOrder = moAcquireRelease): NodePtr =
-  load(node.next, order = moorder)
-
-proc fetchNext*(node: NodePtr, moorder: MemoryOrder = moAcquireRelease): NodePtr =
+proc fetchNext*(node: NodePtr; moorder: MemoryOrder): NodePtr =
   # get the NodePtr to the next Node, can be converted to a TagPtr of (nptr: NodePtr, idx: 0'u16)
-  fetchNext(node.toNode)
+  load(node.toNode.next, order = moorder)
 
-proc fetchAddSlot*(t: var Node, idx: uint16, w: uint, moorder: MemoryOrder = moAcquireRelease): uint =
+proc fetchAddSlot*(t: var Node, idx: uint16, w: uint, moorder: MemoryOrder): uint =
   ## Fetches the pointer to the object in the slot while atomically
   ## increasing the value by `w`.
   ##
@@ -111,11 +108,10 @@ proc fetchAddSlot*(t: var Node, idx: uint16, w: uint, moorder: MemoryOrder = moA
   t.slots[idx].fetchAdd(w, order = moorder)
 
 proc compareAndSwapNext*(t: var Node, expect: var uint, swap: uint): bool =
-  t.next.compareExchange(expect, swap, moRelaxed) # MO as per cpp impl
+  t.next.compareExchange(expect, swap, moRelease, moRelaxed)
 
 proc compareAndSwapNext*(t: NodePtr, expect: var uint, swap: uint): bool =
-  # cpp impl is Relaxed; we use Release here to remove tsan warning
-  (toNode t).next.compareExchange(expect, swap, moRelease)
+  (toNode t).next.compareExchange(expect, swap, moRelease, moRelaxed)
 
 proc `=destroy`*(n: var Node) =
   decDebugCounter()
@@ -137,8 +133,7 @@ proc tryReclaim*(node: var Node; start: uint16) =
     for i in start..<N:
       template s: Atomic[uint] = node.slots[i]
       if (s.load(order = moAcquire) and CONSUMED) != CONSUMED:
-        # cpp impl is Relaxed; we use Release here to remove tsan warning
-        var prev = s.fetchAdd(RESUME, order = moRelease) and CONSUMED
+        var prev = s.fetchAdd(RESUME, order = moRelaxed) and CONSUMED
         if prev != CONSUMED:
           incRecPathCounter()
           break done
@@ -148,9 +143,7 @@ proc tryReclaim*(node: var Node; start: uint16) =
       incReclaimCounter()
 
 proc incrEnqCount*(node: var Node; final: uint16 = 0) =
-  var mask =
-    node.ctrl.fetchAddTail:
-      (final.uint32 shl 16) + 1
+  var mask = node.ctrl.fetchAddTail((final.uint32 shl 16) + 1)
   template finalCount: uint16 =
     if final == 0:
       getHigh mask
@@ -165,9 +158,7 @@ proc incrEnqCount*(node: var Node; final: uint16 = 0) =
 
 proc incrDeqCount*(node: var Node; final: uint16 = 0) =
   incDeqPathCounter()
-  var mask =
-    node.ctrl.fetchAddHead:
-      (final.uint32 shl 16) + 1
+  var mask = node.ctrl.fetchAddHead((final.uint32 shl 16) + 1)
   template finalCount: uint16 =
     if final == 0:
       getHigh mask

loony/spec.nim

@@ -62,14 +62,14 @@ proc getHigh*(mask: ControlMask): uint16 =
 proc getLow*(mask: ControlMask): uint16 =
   mask.uint16
 
-proc fetchAddTail*(ctrl: var ControlBlock, v: uint32 = 1, moorder: MemoryOrder = moRelease): ControlMask =
-  ctrl.tailMask.fetchAdd(v, order = moorder)
+proc fetchAddTail*(ctrl: var ControlBlock, v: uint32 = 1): ControlMask =
+  ctrl.tailMask.fetchAdd(v, order = moRelaxed)
 
-proc fetchAddHead*(ctrl: var ControlBlock, v: uint32 = 1, moorder: MemoryOrder = moRelease): ControlMask =
-  ctrl.headMask.fetchAdd(v, order = moorder)
+proc fetchAddHead*(ctrl: var ControlBlock, v: uint32 = 1): ControlMask =
+  ctrl.headMask.fetchAdd(v, order = moRelaxed)
 
-proc fetchAddReclaim*(ctrl: var ControlBlock, v: uint8 = 1, moorder: MemoryOrder = moAcquireRelease): uint8 =
-  ctrl.reclaim.fetchAdd(v, order = moorder)
+proc fetchAddReclaim*(ctrl: var ControlBlock, v: uint8 = 1): uint8 =
+  ctrl.reclaim.fetchAdd(v, order = moAcquireRelease)
 
 when defined(loonyDebug):
   import std/logging

tests/test.nim

@@ -30,13 +30,14 @@ setLogFilter:
 var q: LoonyQueue[Continuation]
 
 proc runThings() {.thread.} =
-  while true:
-    var job = pop q
-    if job.dismissed:
-      break
-    else:
-      while job.running:
-        job = trampoline job
+  {.gcsafe.}:
+    while true:
+      var job = pop q
+      if job.dismissed:
+        break
+      else:
+        while job.running:
+          job = trampoline job
 
 proc enqueue(c: C): C {.cpsMagic.} =
   check not q.isNil
@@ -61,7 +62,7 @@ else:
     var x = Timespec(tv_sec: 0.Time, tv_nsec: ns)
     var y: Timespec
     if 0 != nanosleep(x, y):
-      raise
+      fail "nanosleep fail"
     c
 
 proc doContinualThings() {.cps: C.} =