Skip to content

STM tests #61

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
Merged
merged 1 commit into from
May 23, 2023
Merged

STM tests #61

merged 1 commit into from
May 23, 2023

Conversation

lyrm
Copy link
Collaborator

@lyrm lyrm commented Mar 6, 2023

Add Multicore.STM tests for :

  • spsc_queue
  • treiber_stack
  • michael_scott_queue
  • mpsc_queue

jmid
jmid reviewed Mar 7, 2023
View reviewed changes
Copy link
Contributor

@jmid jmid left a comment

Choose a reason for hiding this comment

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

Happy to see some STM tests! 😃

I took a pass over this and it LGTM!
I commented on a few minor nits.

test/michael_scott_queue/stm_michael_scott_queue.ml Outdated
| Pop, Res ((Option Int, _), res) -> (
match List.rev s with [] -> res = None | j :: _ -> res = Some j)
| Is_empty, Res ((Bool, _), res) -> (
match s with [] -> res | _ -> not res)
Copy link
Contributor

Choose a reason for hiding this comment

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

This could also be written: res = (s = []) - which is a bit shorter

test/mpsc_queue/stm_mpsc_queue.ml
(is_closed, if not is_closed then i :: List.rev s |> List.rev else s)
| Push_head i -> (is_closed, if not (is_closed && s = []) then i :: s else s)
| Is_empty -> (is_closed, s)
| Pop -> ( (is_closed, match s with [] -> s | _ :: s' -> s'))
Copy link
Contributor

Choose a reason for hiding this comment

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

double parens here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is actually done by ocamlformat. Pretty weird. I raised an issue in ocamlfomat repo.

test/mpsc_queue/stm_mpsc_queue.ml Outdated
| Is_empty, Res ((Result (Bool, Exn), _), res) ->
if is_closed && s = [] then res = Error Mpsc_queue.Closed
else if s = [] then res = Ok true
else res = Ok false
Copy link
Contributor

Choose a reason for hiding this comment

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

these two lines can we written as else res = Ok (s = []) I think

test/treiber_stack/stm_treiber_stack.ml Outdated
| Pop, Res ((Option Int, _), res) -> (
match s with [] -> res = None | j :: _ -> res = Some j)
| Is_empty, Res ((Bool, _), res) -> (
match s with [] -> res | _ -> not res)
Copy link
Contributor

Choose a reason for hiding this comment

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

Or res = (s=[])

@lyrm
Copy link
Collaborator Author

lyrm commented Mar 8, 2023

Thanks for the review ! Some STM tests (like spsc_queue's ones) are pretty slow, any idea why ?

@jmid
Copy link
Contributor

jmid commented Mar 8, 2023

Thanks for the review ! Some STM tests (like spsc_queue's ones) are pretty slow, any idea why ?

Yes, I can see that one is consistently slow in the parallel version across the 4 GitHub CI runs.
Nothing stands immediately out to me in the code though... 🤔
There will be quite some List.reving - but so do the other tests (and they aren't taking as long).

One suggestion could be to run a quick and dirty sudo perf top (if you are on Linux) to get a sense of where time is being spent. You can also do a focused sudo perf top <pid> and even perf record to store the observations for separate analysis...

@jmid
Copy link
Contributor

jmid commented Mar 14, 2023

OK, I spent a bit of time digging into this.
It turns out to be caused by a too optimistic par_len parameter, causing the exponential interleaving search to blow too much up. I fixed this in ocaml-multicore/multicoretests#55 for STM - and apply in 4655f69 the same fix here: reducing the maximum list length of parallel commands from 15 to 12.

@jmid
Copy link
Contributor

jmid commented Mar 14, 2023

If the data structures are OK with having tests run in 3 different domains (creator/seq.prefix, producer, consumer) using the exported WSDT_dom.agree_prop_par property could save quite a few lines by omitting a locally defined agree_prop_par.

@jmid
Copy link
Contributor

jmid commented Mar 15, 2023

Come to think of it, we should add something like agree_prop_par_asym to qcheck-STM as that would reduce much of this customization to just one-liners... 😄

@jmid jmid mentioned this pull request Mar 15, 2023
Closed
@jmid jmid mentioned this pull request Mar 23, 2023
Merged
@jmid
Copy link
Contributor

jmid commented Apr 21, 2023

The PR ocaml-multicore/multicoretests#315 to add agree_prop_par_asym has been merged, so I've now taken a pass over this PR to reduce boilerplate test code, which has resulted in a nice reduction 😃

I've included

  • a similar reduction of the existing ws_deque test
  • a pin of the development version of qcheck-stm until the next opam release

@lyrm lyrm force-pushed the stm-test branch 3 times, most recently from b7a78e9 to 639c4d2 Compare May 3, 2023 17:49
@lyrm
Copy link
Collaborator Author

lyrm commented May 3, 2023

Mmmh, I removed the pin in the .opam file on qcheck-stm because I thought it was working without it, but it does not.

@jmid : any idea when a new version of multicoretests will be released with theses changes ?

@jmid
Copy link
Contributor

jmid commented May 4, 2023

Mmmh, I removed the pin in the .opam file on qcheck-stm because I thought it was working without it, but it does not.

Indeed. The added bindings are not available in the previous release, hence the pin.
This is a one-liner change once I roll release, so I don't see why this should hold back the PR.

@jmid : any idea when a new version of multicoretests will be released with theses changes ?

Perhaps next week. I'm currently trying to push out a new release of the underlying QCheck first, but opam-ci is on its knees. If my luck continues like this, you may have to wait longer.

@lyrm lyrm merged commit 7195de5 into ocaml-multicore:main May 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmid jmid jmid left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@lyrm @jmid