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

Switch STM_domain.agree_prop_par_asym to use Atomics #368

Merged
merged 3 commits into from
Jun 21, 2023
Merged

Conversation

jmid
Copy link
Collaborator

@jmid jmid commented Jun 16, 2023

I just got around to running a bit of statistics on STM_domain.agree_prop_par_asym for the ref int test.

Switching to use Atomic looks like a clear win when testing locally.
Over 10000 runs this represents a provable improvement at 95% confidence both when compiling to byte code:

$ dune exec src/statistics/z_test.exe 10000 317 10000 786
z-test of two proportions         
z = -14.527927
Is |z| = |-14.527927| > z_alpha2 = 1.960000 ?
Yes, null hypothesis rejected

and when compiling to native code:

$ dune exec src/statistics/z_test.exe 10000 785 10000 1879
z-test of two proportions         
z = -22.766211
Is |z| = |-22.766211| > z_alpha2 = 1.960000 ?
Yes, null hypothesis rejected

In both cases, the number of failed properties doubled!
Interestingly, native code is significantly better at causing trouble than bytecode.

Hopefully this should take care of #364

@jmid
Copy link
Collaborator Author

jmid commented Jun 20, 2023

Whereas the stats are true for Linux, they don't hold up on macOS:

  • a macOS trunk 5.2 run fails to trigger STM int64 ref test parallel asymmetric in 2000 iterations

Other CI results include

@jmid
Copy link
Collaborator Author

jmid commented Jun 20, 2023

I ran some stats on my old macBook yesterday:

  • with the existing Semaphore.Binary (2-3 runs with 10000 iterations)
    - native asym 54/73/59
    - bytecode asym 17/22/31
  • with the proposed Atomic bool
    - native asym 71/86
    - bytecode asym 201/170
  • with an Atomic int
    - native asym 131/119
    - bytecode asym 260/246

So, an Atomic bool is better than a Semaphore.Binary on macOS with native code:

$ dune exec src/statistics/z_test.exe 30000 186 20000 157
z-test of two proportions         
z = -2.189814
Is |z| = |-2.189814| > z_alpha2 = 1.960000 ?
Yes, null hypothesis rejected

and an Atomic int is better still:

$ dune exec src/statistics/z_test.exe 20000 157 20000 250
z-test of two proportions         
z = -4.633472
Is |z| = |-4.633472| > z_alpha2 = 1.960000 ?
Yes, null hypothesis rejected

And the same holds for under bytecode.

@jmid
Copy link
Collaborator Author

jmid commented Jun 20, 2023

CI summary:

Overall, this is encouraging as the different kinds of failures are getting fewer - and none of them is an asym failure.

@jmid
Copy link
Collaborator Author

jmid commented Jun 20, 2023

I've added a CHANGES entry in 840a01d as this changes the user-facing STM_domain.agree_prop_par_asym binding for the better.
I've also rerun the lockfree test suite (our primary client) with the result to confirm that it continues to work as expected.
I'll let the new CI run complete over night and then merge tomorrow if things continue to look good.

@jmid
Copy link
Collaborator Author

jmid commented Jun 21, 2023

CI summary:

Again, no issues with the adjusted STM_domain.agree_prop_par_asym from this PR show up, so I'll merge.

@jmid jmid merged commit 1be22ae into main Jun 21, 2023
43 of 47 checks passed
@jmid jmid deleted the asym-atomic branch June 21, 2023 08:51
@jmid jmid linked an issue Jun 21, 2023 that may be closed by this pull request
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.

'STM _ ref test parallel asymmetric' failure to trigger
1 participant