Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add a garbage collector to task queue #29

Open
wants to merge 5 commits into
base: stable
Choose a base branch
from
Open

Add a garbage collector to task queue #29

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4774e94
Add a garbage collector to task queue
mratsim Feb 6, 2023
cd6a2ad
Merge branch 'stable' into gc
arnetheduck Jul 5, 2023
7c0395c
Update taskpools/chase_lev_deques.nim
arnetheduck Oct 24, 2023
a42345a
Merge branch 'stable' into gc
arnetheduck Jan 2, 2024
a0d21ad
Update taskpools/chase_lev_deques.nim
arnetheduck Jan 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 16 additions & 10 deletions taskpools/chase_lev_deques.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tp_allocUnchecked comment still applies, even with the renamed field.

# Unused. There is no memory reclamation scheme.
prev: ptr Buf[T]
prevRetired: ptr Buf[T] # intrusive linked list. Used for garbage collection
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


capacity: int
mask: int # == capacity-1 implies (i and mask) == (i mod capacity)
Expand Down Expand Up @@ -87,7 +85,7 @@ proc newBuf(T: typedesc, capacity: int): ptr Buf[T] =
zero = true
)

# result.prev = nil
result.prevRetired = nil
result.capacity = capacity
result.mask = capacity - 1
# result.rawBuffer.addr.zeroMem(sizeof(T)*capacity)
Expand All @@ -111,13 +109,21 @@ proc grow[T](deque: var ChaseLevDeque[T], buf: var ptr Buf[T], top, bottom: int)
for i in top ..< bottom:
tmp[][i] = buf[][i]

buf.prev = deque.garbage
buf.prevRetired = deque.garbage
deque.garbage = buf
# publish globally
deque.buf.store(tmp, moRelaxed)
# publish locally
swap(buf, tmp)

proc garbageCollect(deque: var ChaseLevDeque) {.inline.} =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
proc garbageCollect(deque: var ChaseLevDeque) {.inline.} =
proc garbageCollect(deque: var ChaseLevDeque) =

there's no difference between inline and not on private functions

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

var node = deque.garbage
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

while node != nil:
let tmp = node.prevRetired
c_free(node)
node = tmp
deque.garbage = nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does garbage need to be Atomic as well?

(Doesn't seem like it's necessary to me, but still worth to double-check intent)


# Public API
# ---------------------------------------------------

Expand All @@ -128,11 +134,7 @@ proc init*[T](deque: var ChaseLevDeque[T], initialCapacity: int) =

proc teardown*[T](deque: var ChaseLevDeque[T]) =
## Teardown a Chase-lev work-stealing deque.
var node = deque.garbage
while node != nil:
let tmp = node.prev
c_free(node)
node = tmp
deque.garbageCollect()
c_free(deque.buf.load(moRelaxed))

proc push*[T](deque: var ChaseLevDeque[T], item: T) =
Expand All @@ -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:
arnetheduck marked this conversation as resolved.
Show resolved Hide resolved
# Empty queue, no thieves can have a pointer to an old retired buffer
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assumption seems correct to me, if steal copies rawBuffer.

If steal were lent instead, the c_free(node) could mess with it, right?

deque.garbageCollect()

a[][b] = item
fence(moRelease)
deque.bottom.store(b+1, moRelaxed)
Expand Down
Loading