Add a garbage collector to task queue #29
Conversation
The per-threadtask queues grows if it can not fit all pending tasks in queue but never release the extra memory buffer. In practice it should have no impact on Nimbus, it would require a task to `spawn`a lot of tasks to fill the default queue (32 tasks) and on Nimbus it is only used for parallelizing batch signature verification, which spawns as many tasks as there are cores.
| deque.garbage = buf | ||
| # publish globally | ||
| deque.buf.store(tmp, moRelaxed) | ||
| # publish locally | ||
| swap(buf, tmp) | ||
|
|
||
| proc garbageCollect(deque: var ChaseLevDeque) {.inline.} = |
There was a problem hiding this comment.
| proc garbageCollect(deque: var ChaseLevDeque) {.inline.} = | |
| proc garbageCollect(deque: var ChaseLevDeque) = |
there's no difference between inline and not on private functions
There was a problem hiding this comment.
Actually there is, you can access private functions and variables from another module, using templates for example.
I'm not too sure how strong a hint visibility((hidden)) is to the compiler but it's definitely less than C static (only accessible from this module) and static inline.
Co-authored-by: Mamy Ratsimbazafy <[email protected]>
| let tmp = node.prevRetired | ||
| c_free(node) | ||
| node = tmp | ||
| deque.garbage = nil |
There was a problem hiding this comment.
Does garbage need to be Atomic as well?
(Doesn't seem like it's necessary to me, but still worth to double-check intent)
| deque.garbage = buf | ||
| # publish globally | ||
| deque.buf.store(tmp, moRelaxed) | ||
| # publish locally | ||
| swap(buf, tmp) | ||
|
|
||
| proc garbageCollect(deque: var ChaseLevDeque) {.inline.} = |
There was a problem hiding this comment.
In grow, there is a comment referring to garbageUsed, but that garbageUsed doesn't seem to exist. Is there something missing, or should the comment be cleaned up?
| @@ -48,9 +48,7 @@ type | |||
| ## Backend buffer of a ChaseLevDeque | |||
| ## `capacity` MUST be a power of 2 | |||
|
|
|||
| # Note: update tp_allocUnchecked allocation if any field changes. | |||
There was a problem hiding this comment.
The tp_allocUnchecked comment still applies, even with the renamed field.
| @@ -148,6 +150,10 @@ proc push*[T](deque: var ChaseLevDeque[T], item: T) = | |||
| # Full queue | |||
| deque.grow(a, t, b) | |||
|
|
|||
| if not deque.garbage.isNil and b == t: | |||
| # Empty queue, no thieves can have a pointer to an old retired buffer | |||
There was a problem hiding this comment.
Assumption seems correct to me, if steal copies rawBuffer.
If steal were lent instead, the c_free(node) could mess with it, right?
| # Note: update tp_allocUnchecked allocation if any field changes. | ||
| # Unused. There is no memory reclamation scheme. | ||
| prev: ptr Buf[T] | ||
| prevRetired: ptr Buf[T] # intrusive linked list. Used for garbage collection |
There was a problem hiding this comment.
Maybe prevGarbage, or even just garbage to keep nomenclature consistent, instead of using retired and garbage for the same concept.
also not sure about intrusive in comment.
| deque.garbage = buf | ||
| # publish globally | ||
| deque.buf.store(tmp, moRelaxed) | ||
| # publish locally | ||
| swap(buf, tmp) | ||
|
|
||
| proc garbageCollect(deque: var ChaseLevDeque) {.inline.} = | ||
| var node = deque.garbage |
There was a problem hiding this comment.
If garbageCollect takes the Buf[T] instead of the ChaseLevDeque, I think the extra garbage in ChaseLevDequeue would no longer be necessary. We could simply start here at buf.prevGarbage instead. garbageCollect is only called when no thieves are present, by the owner, so it should be safe imo.
Co-authored-by: Etan Kissling <[email protected]>
The per-thread task queues grows if it can not fit all pending tasks in queue but never release the extra memory buffer.
In practice it should have no impact on Nimbus, it would require a task to
spawna lot of tasks to fill the default queue (32 tasks) and on Nimbus it is only used for parallelizing batch signature verification, which spawns as many tasks as there are cores.