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.

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

Add simple examples for MS queue #59

Open
wants to merge 37 commits into
base: main
Choose a base branch
from

Conversation

nikochiko
Copy link

This pull request adds a simple example for the MS queue

@nikochiko nikochiko marked this pull request as draft March 1, 2023 09:29
@nikochiko
Copy link
Author

nikochiko commented Mar 1, 2023

@Sudha247 I have added a simple example with 3 domains, each performing one task that does push-work-pop, and prints what was pushed and popped.

Is this a good example and the expected usage?

Do you have some other cases in mind that might make for good examples?

Copy link
Collaborator

@Sudha247 Sudha247 left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @nikochiko! This is a great start, leaving some suggestions below -

  • We can perhaps start with something simpler; just a single push and a pop.
  • Then proceed to show concurrent pushes, concurrent pops, and later a combination of both.

Comment on lines 15 to 18
let d1 = Domain.spawn(fun _ -> push_work_pop ms_q 1) in
let d2 = Domain.spawn(fun _ -> push_work_pop ms_q 2) in
let d3 = Domain.spawn(fun _ -> push_work_pop ms_q 3) in
Domain.join d1; Domain.join d2; Domain.join d3
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is correct, we can use Arrays or Lists to make this easier to manage --

let domains = Array.init 4 (fun _ -> Domain.spawn(...)) in
...
Array.iter Domain.join domains

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, this looks much better yes.

@nikochiko nikochiko marked this pull request as ready for review March 1, 2023 21:59
@nikochiko
Copy link
Author

nikochiko commented Mar 1, 2023

@Sudha247 Thanks for the review.

I've pushed a fix with some more examples.

  • Each example is organised as a self-contained function from the creation of queue till end of all operations
  • A printf is called every time an item is pushed/popped. I added it so that it will give some assurance when run that things are happening correctly, but I'm not sure about it because it feels distracting while reading the code
  • In the last example with concurrent push and pop both, n_domains/2 domains are created for both pushers and poppers, the popper is a recursive function that pops exactly one item.
  • In the last example, the popper domains are spawned before pusher domains so that it is apparent from the logs that poppers and pushers are both working at the same time. But I am not sure about this because it might make it a little confusing for the reader.

@Sudha247
Copy link
Collaborator

Sudha247 commented Mar 3, 2023

This is covering more functionality now. I like how the examples are self contained - it will be easy to move this to another format of documentation/tutorial if needed (not needed now).

A printf is called every time an item is pushed/popped. I added it so that it will give some assurance when run that things are happening correctly, but I'm not sure about it because it feels distracting while reading the code

I agree, we don't need print statements for all functions.

In the last example, the popper domains are spawned before pusher domains so that it is apparent from the logs that poppers and pushers are both working at the same time. But I am not sure about this because it might make it a little confusing for the reader.

The popper explicitly mentions that it's going to keep looping until it pops an element. So, I think this is okay.

@lyrm
Copy link
Collaborator

lyrm commented Mar 3, 2023

Nice work, thanks !

Here is a few idea that could make good use of your prints :

  • make domains push or pop different values or write their own id so it is possible to see in which order they have worked. Domain.self is not great for that, but you can assign an integer to each domain when spawning them.

  • adding a do_work function that gives a chance to domains to work in a non sequential order (you can use Domain.cpu_relax in a loop for example) .

polytypic and others added 9 commits March 10, 2023 14:41
…ix-and-tweaks

Fix space leaks of Michael-Scott queue and avoid the impossible
I realized my previous attempt suffered from pretty much the same race condition
as the earlier implementations.

The logic of the new tail update algorithm is as follows:

> The tail update is only responsible for updating the tail as long as the new
> node added still has the true tail. At any point some newly added node has the
> true tail, so this responsibility ensures that the tail gets updated. To check
> whether we are still responsible, we compare: `get new_tail == Nil`. If that
> is false, then someone else is responsible and we can stop. The current
> old_tail must be read before that check. If the `compare_and_set` fails, we
> must retry. Otherwise we can stop as we've done our part.
Set QCHECK_MSG_INTERVAL to avoid clutter in CI logs
Mark alcotest as a test dependency
Add better print statements, accompanied by a pusher/popper id.
Add a do_work function that calls Domain.cpu_relax a random large
number of times. This makes concurrency apparent after the example
is run.
@nikochiko
Copy link
Author

Thanks for the feedback.

I have added the changes we discussed. Removed some print statements, the ones that are there will log with a pusher/popper ID. do_work runs Domain.cpu_relax a random large number of times, so that the concurrency is apparent when the examples are run.

…ue-tail-update

Fix the lock-free update of Michael-Scott style queue tail
@bartoszmodelski bartoszmodelski linked an issue May 10, 2023 that may be closed by this pull request
@bartoszmodelski bartoszmodelski changed the title #57: Add simple examples for MS queue Add simple examples for MS queue May 10, 2023
Copy link
Contributor

@bartoszmodelski bartoszmodelski left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@bartoszmodelski bartoszmodelski left a comment

Choose a reason for hiding this comment

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

lgtm

@nikochiko
Copy link
Author

Thanks for the review.
Done and I also ran it through dune fmt because that check was failing.

Sudha247 and others added 3 commits May 11, 2023 10:32
This code of conduct lives in [ocaml/code-of-conduct](https://github.com/ocaml/code-of-conduct) and has been discussed in [this thread](https://discuss.ocaml.org/t/ocaml-community-code-of-conduct/10494).

It has been adopted by the OCaml compiler and many platform tools. After a discussion we propose adopting it for lockfree.
lyrm and others added 21 commits May 22, 2023 09:46
- Change data structures names for user-ergonomy (treiber_stack -> stack for example)
- Uniformize queue interface and doc
- Add CONTRIBUTING file and complete test/README.md
Refactoring to have a lockfree package
Rename `Lockfree` module in `saturn_lockfree` package to `Saturn_lockfree`.
Add a barrier module in `tests` to replace the use of `Semaphore`.
@Sudha247
Copy link
Collaborator

I've rebased the PR to reflect updates in the repo and the name change. Will merge once the CI is happy.

@lyrm lyrm mentioned this pull request Jan 24, 2024
12 tasks
@Sudha247 Sudha247 added this to the 1.0 milestone Jan 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add examples
7 participants