-
Notifications
You must be signed in to change notification settings - Fork 31
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.
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
Lock free skiplist with size #99
Conversation
c6d1d45
to
6e9f4d7
Compare
9769875
to
581898f
Compare
4b61460
to
c2adcc8
Compare
d09d0d5
to
8d448f7
Compare
src_lockfree/skiplist.ml
Outdated
if level = 0 then Size.update_once t.size r.decr; | ||
find_path t key prev prev_at_level preds succs level lowest | ||
(let after = Link r.node in | ||
if Atomic.compare_and_set prev_at_level before after then after | ||
else Atomic.get prev_at_level) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line made me think quite a bit so I'm leaving a note in hope it may help someone else (or a future version of myself).
I was wondering why the recursive call here is made at the current level and not at a higher level as if curr_node
has higher levels there are also marked but these marked could have been missed by the current find_path
call. Also, I made the wrong assumption that these missed marks may cause the function calling find_path
(add
for example) to do some costlier operations to remove the marked node. So I was thinking maybe we can prevent it by looping a bit higher here (basically at the max height of the marked node, if possible).
But :
- the
add
function actually does not make an avoidable costlier operation : either the node has been completely removed making aCAS
fail (line 263) (this is unavoidable as the removal operation can be completed by almost all operations) or the removal operation is not finished andadd
can finish without any additional work - it may actually make thing worst as encountering a marked node may happen repetitively. So if each time this happen
find_path
goes back up a few levels it may be delayed indefinitely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. Hmm... Yes, I wonder whether there might be ways to do these traversals more intelligently.
🤔
I think a reasonable basic assumption is that encountering a marked link is something that should happen relatively rarely. That is because marked links are only used on nodes to be removed and so usually you would expect a particular marked link to be encountered roughly just once — unless there is heavy ongoing parallelism — and even in that case only a single thread will succeed in unlinking the node at each level. Also, the expected number of links (and therefore the number of marked links) on a node is (less than) two, because of the way the node heights are distributed. So, it is probably better to optimize with the assumption that marked links are rare.
if Atomic.compare_and_set next before succ then | ||
update_nexts (level' - 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see a reason to not do an Atomic.set
here instead of and Atomic.compare_and_set
as no one can access these levels of the added node yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... It is a subtle thing, but after the node has been added to level 0, i.e. the first compare_and_set
in add
has succeeded, the node is considered as having been added to the skiplist and then remove
may concurrently find and mark all the outgoing links from the node. The remove
operation is special in this sense. Other operations cannot access outgoing links of a node on levels that the node has not yet been linked to from other nodes of the skiplist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, right, the node can be marked. The tests passed on my computer with this change so there is definitely a dscheck
test missing here.
d5e5df8
to
f81349e
Compare
0cb133e
to
c86f96c
Compare
d118822
to
e858099
Compare
5114f01
to
877bee5
Compare
21985d6
to
87dca1f
Compare
87dca1f
to
f642273
Compare
This adds a minimalistic optimized lock-free skiplist implementation with size (using and written on top of PR #102).
For future work:
The benchmark in this PR is not ideal, but I plan to leave that to (near) future work. I've recently enhanced the small internal multicore benchmarking framework I've been using in Kcas and the plan is to move that to a separate package to allow using it in other projects (like Saturn).