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

Some fixes discovered on high usage of Miou.call and Miou.Queue #36

Merged
merged 3 commits into from
Aug 17, 2024
Merged

Some fixes discovered on high usage of Miou.call and Miou.Queue #36

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a17fc2d
Don't use == which can lead to a segfault due to optimisation
dinosaure Aug 11, 2024
2b4be1d
Fix the mpsc queue and prefer to use a snapshot instead of walking in…
dinosaure Aug 11, 2024
9d18413
Use Int.equal instead of ==
dinosaure Aug 11, 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
2 changes: 1 addition & 1 deletion lib/miou_gen.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ module Make () = struct
let null = 0
let pp = Format.pp_print_int
let compare a b = a - b
let equal a b = a == b
let equal = Int.equal

external of_int : int -> t = "%identity"
external to_int : t -> int = "%identity"
Expand Down
22 changes: 8 additions & 14 deletions lib/miou_queue.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,8 +107,7 @@ let length t =
let head, tail = snapshot t in
tail.count - head.count

let iter ~f t =
let head, tail = snapshot t in
let iter ~f (head, tail) =
let rec go prev =
if prev != tail then
match Atomic.get prev.next with
Expand All @@ -117,18 +116,13 @@ let iter ~f t =
in
go head

let rec drop ~f t =
let head, tail = snapshot t in
if Atomic.compare_and_set t.head head tail then (
let rec go prev =
if prev != tail then
match Atomic.get prev.next with
| None -> ()
| Some next -> f next.value; go next
in
go head;
tail.value <- Obj.magic ())
else drop ~f t
let rec drop t =
let ((head, tail) as snapshot) = snapshot t in
if Atomic.compare_and_set t.head head tail
then snapshot else drop t
Comment on lines +119 to +122
Copy link
Contributor

Choose a reason for hiding this comment

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

This has a space leak. The problem is that, unless the queue is already empty, the node pointed to by tail still has a value and after a successful compare_and_set that value will still be pointed to by the queue.

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, that's the bug I encountered on another project. I'm not sure how to implement a atomic drop unfortunately. I decided to switch to a mutex + a simple queue.


let drop ~f t = iter ~f (drop t)
let iter ~f t = iter ~f (snapshot t)
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that iter is not concurrency safe. If one thread calls iter ~f q and another thread performs dequeue q, then the ~f function may be called with Obj.magic ():

miou/lib/miou_queue.ml

Line 63 in 9d18413

next.value <- Obj.magic ();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

More precisely, this is what I encountered on my project. I suspected that this version will help me but it's not - and it's why I decided to switch to a mutex + a simple queue. If you have an idea to have an atomic iter, it will be superb but my brain is actually focus and limited on some others stuffs.


let to_list t =
let res = ref [] in
Expand Down
4 changes: 2 additions & 2 deletions lib/miou_sync.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,8 @@ end = struct
| Awaiting (fn, x, y) as seen ->
if Atomic.compare_and_set t seen Signaled then fn t x y else signal t

let is_signaled t = Atomic.get t == Signaled
let is_initial t = Atomic.get t == Initial
let is_signaled t = Atomic.get t = Signaled
let is_initial t = Atomic.get t = Initial
Comment on lines +55 to +56
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that == and = will do the exact same thing in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but I prefer to be conservative on this space. I mean, it's hard to remember all reasons that it's currently safe to user == instead of = 😕.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that Atomic.compare_and_set uses physical equality. I would generally recommend thinking in terms of physical equality with atomics unless there is very specific reason to prefer other forms of equality.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is indeed the model I could have had in the first place, but this issue still makes me break out in a cold sweat over the use of ==. Once again, the point is to facilitate rereading without reconsidering why it's okay to use == 10 years after you've done it!

let[@inline never] awaiting () = invalid_arg "Trigger: already awaiting"

let rec on_signal t x y fn =
Expand Down