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

Refactor IOSimPOR #114

Merged
merged 28 commits into from
Sep 27, 2023
Merged

Refactor IOSimPOR #114

merged 28 commits into from
Sep 27, 2023

Conversation

coot
Copy link
Collaborator

@coot coot commented Sep 21, 2023

A long list of self contained patches. See CHANGELOG.md for a short description of all the changes.

@coot coot force-pushed the coot/refactor-io-sim-por branch 4 times, most recently from a71aef7 to 135e42a Compare September 23, 2023 14:13
@coot
Copy link
Collaborator Author

coot commented Sep 23, 2023

With unsafeInterleaveST

conjoinParST :: TestableNoCatch prop => [ST s prop] -> ST s Property
conjoinParST sts = do
    ps <- sequence (unsafeInterleaveST <$> sts)
    return $ conjoinPar ps
N Total Elapsed MUT MUT Elapsed
1 74.625s 74.520s 59.564s 59.423s
2 93.936s 58.870s 71.665s 44.233s
3 103.538s 55.560s 78.795s 42.647s
4 111.185s 54.310s 83.378s 41.874s
5 113.673s 53.420s 84.926s 41.594s
6 119.478s 53.920s 88.528s 42.048s
7 124.710s 54.120s 92.008s 42.428s
8 119.583s 51.430s 88.580s 40.912s
9 129.239s 53.010s 94.851s 42.084s
10 126.762s 52.070s 93.471s 41.733s
11 130.695s 52.080s 95.989s 41.832s
12 136.149s 52.570s 99.176s 42.127s
-- ---------- --------- ---------- -------------
24 177.372s 58.281s 123.908s 46.076s

Note: on a machine ith 12 physical cores, 24 CPU threads.

Without unsafeInterleaveST

conjoinParST :: TestableNoCatch prop => [ST s prop] -> ST s Property
conjoinParST sts = do
    ps <- sequence sts
    return $ conjoinPar ps
N Total Elapsed MUT MUT Elapsed
1 84.907s 84.790s 62.006s 61.850s
2 101.086s 89.570s 69.277s 69.025s
3 104.942s 88.220s 70.331s 69.479s
4 106.666s 86.870s 70.994s 69.499s
5
6 108.294s 85.110s 72.338s 69.597s
7
8
9
10 121.869s 86.000s 75.760s 70.053s
11
12 129.112s 86.470s 77.958s 70.382s

@coot coot marked this pull request as ready for review September 25, 2023 08:35
@dcoutts
Copy link
Contributor

dcoutts commented Sep 25, 2023

Any idea why the with/without unsafeInterleaveST is 10s different @ -N1? 74 vs 84s @ -N1

@coot
Copy link
Collaborator Author

coot commented Sep 25, 2023

No, it's a bit surprising. When I run it now I get similar results. Different test case, and previously I was running with profiling, this time without. Still there's a 10% difference.

Par

N Total Elapsed
1 7.529 7.530
6 11.648 5.610
12 13.709 5.550

Seq

N Total Elapsed
1 8.177 8.170
6 11.513 8.700
12 12.646 8.400

Copy link
Contributor

@bolt12 bolt12 left a comment

Choose a reason for hiding this comment

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

Very nice! Just a couple of comments

io-sim/src/Control/Monad/IOSim.hs Outdated Show resolved Hide resolved
io-sim/src/Control/Monad/IOSim.hs Show resolved Hide resolved
io-sim/src/Control/Monad/IOSim.hs Outdated Show resolved Hide resolved
io-sim/test/Test/Control/Monad/IOSimPOR.hs Outdated Show resolved Hide resolved
io-sim/test/Test/Control/Monad/IOSimPOR.hs Outdated Show resolved Hide resolved
io-sim/src/Control/Monad/IOSimPOR/Types.hs Outdated Show resolved Hide resolved
io-sim/test/Test/Control/Monad/IOSimPOR.hs Show resolved Hide resolved
io-sim/CHANGELOG.md Outdated Show resolved Hide resolved
Added `BlockedOnDelay` and `BlockedOnThrowTo` instead of
`BlockedOnOther`.
Instead of using `forkIO` & `threadDelay` it's more robust to test with
`async` and `wait`.
This allows to show more compact information about execution.  It's
useful for debugging IOSimPOR.
We report `EventTxWakup` in the thread that waken up the thread, but we
report the `ThreadId` of the awaken thread.  Including step of the
executed thread is misleading.
This is only useful for debugging `IOSimPOR` issues.
The original type clashes with `ThreadId` type class from `io-classes`
which is in particular annoying when pasting `ControlSchedules` in
`ghci` or counterexamples.
Consider the following sequence of events

x
⋮
y
⋮
z

If x is racing with z, we will reverse x and z (e.g. postpone x after
z).  If y is also racing with x, then we should not include in the
schedule any event which happens after it.  Originally we used a policy
to remove y's thread from the list of racing threads with the x's
thread.  But this turns out to be too week.  While we are visiting new
events we need to remove their threads as soon as the event happens
after any of the so far discovered races.  Otherwise we can end with
a schedule which contains events that can only happen after the racing
event was executed, and thus likely will block and violate internal
invariant: all steps which we follow should be in the runqueue (e.g.
should not be blocked).
Note: `deschedule` is traced by the caller, not by itself.
This makes it much easier to debug ones.  In the future we might turn
more assertion failures into `FailureInternal`.
Unlike `exploreSimTrace` it allows to run an `ST` action in the context
of each schedule, and thus allows to share state between schedules.  For
this reason it evaluates each schedule sequentially, while
`exploreSimTrace` evaluates each schedule in parallel.

This patch also makes the API more uniform by exposing:
* `exploreSimTrace` & `exploreSimTraceST`
* `controlSimTrace` & `controlSimTraceST`

This patch also removes parallel evaluation of different schedules.

Furthermore the tests are updated and they no longer use
`unsafePerformIO` as well.  The `traceCounter` property was removed as
it wasn't used and doesn't make much sense, since `IOSimPOR` is forcing
to evaluate each schedule only once by using a cache.
This should be exposed as part of the `newTimeout` API.
This can be added to `.hlint.yaml` file (we do not checkout it to the
git repo, but one can add it to one of the `.gitignore` files).
@coot
Copy link
Collaborator Author

coot commented Sep 26, 2023

@bolt12 I addressed your review. I accepted most of your comments, the one that I challenged I left opened.

@coot coot added this pull request to the merge queue Sep 27, 2023
Merged via the queue into main with commit 892076e Sep 27, 2023
13 of 14 checks passed
@coot coot deleted the coot/refactor-io-sim-por branch September 27, 2023 12:46
@coot coot mentioned this pull request Oct 4, 2023
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.

None yet

3 participants