RFR: 7902982: jcstress: Add samples for some mutex algorithms [v2]

[Michael Mirwaldt]

On Mon, 28 Jun 2021 12:41:07 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

>> mmirwaldt for openjdk has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   improved code by applying ideas from the PR
>
> jcstress-samples/src/main/java/org/openjdk/jcstress/samples/concurreny/mutex/Mutex_01_NoAlgorithm.java line 49:
> 
>> 47:      */
>> 48:     @JCStressTest
>> 49:     @Outcome(id = {"1, 1", "1, 2", "2, 1", "2, 2"}, expect = ACCEPTABLE, desc = "Both actors can enter the critical section one by one but must not")
> 
> But must not what? The description seems to end prematurely.

Sentence completed

> jcstress-samples/src/main/java/org/openjdk/jcstress/samples/concurreny/mutex/Mutex_01_NoAlgorithm.java line 51:
> 
>> 49:     @Outcome(id = {"1, 1", "1, 2", "2, 1", "2, 2"}, expect = ACCEPTABLE, desc = "Both actors can enter the critical section one by one but must not")
>> 50:     @State
>> 51:     public static class NoAlgorithm {
> 
> Since there is only one test case in this file, you can move everything to the top class.

Done

> jcstress-samples/src/main/java/org/openjdk/jcstress/samples/concurreny/mutex/Mutex_01_NoAlgorithm.java line 61:
> 
>> 59:             } else {
>> 60:                 r.r1 = 2;
>> 61:             }
> 
> This kind of code introduces its own synchronization, so it might hide some outcomes. I believe you want to demonstrate the "already taken" detection code works? If so, I suggest doing it this way:
> 
> 
> boolean taken1, taken2;
> 
> @Actor
> public void actor1(ZZ_Result r) {
>     taken1 = true;
>     r.r1 = taken2;
>     taken1 = false;
> }
> 
> @Actor
> public void actor2(ZZ_Result r) {
>     taken2 = true;
>     r.r2 = taken1;
>     taken2 = false;
> }

Yes, I tried to find a way to find out whether both actors have entered the critical section at the same time.
Your solution is brilliant! At least one of both actors will always witness in the result that both actors have entered the critical section if they did. However, I think the booleans taken1 and taken2 must be volatile because 
a) they run in different threads with their local caches and might not notice changes of the booleans from the other thread
b) volatile forces the order of those 3 statements. Otherwise, the JVM is allowed to change the order in which they are executed.
Am I right or wrong with volatile here? 
If I am wrong, I will remove the volatile again, of course.

> jcstress-samples/src/main/java/org/openjdk/jcstress/samples/concurreny/mutex/Mutex_02_PetersonAlgorithm.java line 64:
> 
>> 62:             flagForActor1.set(true);
>> 63:             turn.set(2);
>> 64:             while (flagForActor2.get() && turn.get() == 2) ;
> 
> Here and later. Excess whitespace before ` ;`?

I didn't add the whitespace on purpose. IntelliJ added it when I applied code reformatting.
I wouldn't change it because I guess most developers who use IntelliJ just use the default code format.
And if they apply code formatting  (as I use all the time), the whitespace will accidentally added again anyway.
What do you think about that?

> jcstress-samples/src/main/java/org/openjdk/jcstress/samples/concurreny/mutex/Mutex_02_PetersonAlgorithm.java line 70:
> 
>> 68:             } else {
>> 69:                 r.r1 = 2;
>> 70:             }
> 
> Here and later. Replace with new code for "taken" detection?

I have replaced my solution with yours in all 3 samples.

> jcstress-samples/src/main/java/org/openjdk/jcstress/samples/concurreny/mutex/Mutex_03_DekkerAlgorithm.java line 2:
> 
>> 1: /*
>> 2:  * Copyright (c) 2014, 2015, Oracle and/or its affiliates. All rights reserved.
> 
> Same.

Done

> jcstress-samples/src/main/java/org/openjdk/jcstress/samples/concurreny/mutex/Mutex_03_DekkerAlgorithm.java line 52:
> 
>> 50:     @Outcome(id = {"1, 1"}, expect = ACCEPTABLE, desc = "Both actors could enter the critical section")
>> 51:     @Outcome(id = {"1, 2", "2, 1", "2, 2"}, expect = FORBIDDEN, desc = "At least one actor couldn't enter the critical section")
>> 52:     @Outcome(id = {"0, 0", "0, 1", "1, 0", "0, 2", "2, 0"}, expect = FORBIDDEN, desc = "At least one actor hang up in one of the loops")
> 
> Again, can drop `id`.

I am sorry, I don't understand you. Can you write here how the code without id looks like?
I mean I need the id here in the annotations to declare the expected results because I will get an compiler error otherwise.

> jcstress-samples/src/main/java/org/openjdk/jcstress/samples/concurreny/mutex/Mutex_03_DekkerAlgorithm.java line 54:
> 
>> 52:     @Outcome(id = {"0, 0", "0, 1", "1, 0", "0, 2", "2, 0"}, expect = FORBIDDEN, desc = "At least one actor hang up in one of the loops")
>> 53:     @State
>> 54:     public static class DekkerAlgorithm {
> 
> Again, can move to top class.

Done

-------------

PR: https://git.openjdk.java.net/jcstress/pull/85