RFR: CODETOOLS-7903021: jcstress: Dining Philosophers problem sample [v4]

[Michael Mirwaldt]

On Mon, 11 Oct 2021 13:58:26 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

>> Michael Mirwaldt has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   improved comment and ResourceHierarchy solution in ClassicProblem_01_DiningPhilosophers
>
> jcstress-samples/src/main/java/org/openjdk/jcstress/samples/concurrency/classic/ClassicProblem_01_DiningPhilosophers.java line 2:
> 
>> 1: /*
>> 2:  * Copyright (c) 2014, 2015, Oracle and/or its affiliates. All rights reserved.
> 
> Should be `2021`.

Done.

> jcstress-samples/src/main/java/org/openjdk/jcstress/samples/concurrency/classic/ClassicProblem_01_DiningPhilosophers.java line 56:
> 
>> 54:     public static class ResourceHierarchy {
>> 55:         private final Semaphore[] forks =
>> 56:                 new Semaphore[]{new Semaphore(1), new Semaphore(1), new Semaphore(1)};
> 
> These do not need to be `Semaphores`, just plain `Object` locks, I think. `Semaphore` with a single permit is basically a mutex.

Good idea! Done.

> jcstress-samples/src/main/java/org/openjdk/jcstress/samples/concurrency/classic/ClassicProblem_01_DiningPhilosophers.java line 85:
> 
>> 83:                 forks[fork1].acquire();
>> 84:                 forks[fork2].acquire();
>> 85:                 // eating
> 
> Shouldn't this call `tryPickForks`?

No, because the resource hierarchy solution doesn't try to pick a fork. It either picks the fork or it just waits for the fork.
And it can wait for the lock because it can rely on the order of the forks which avoids a deadlock.

> jcstress-samples/src/main/java/org/openjdk/jcstress/samples/concurrency/classic/ClassicProblem_01_DiningPhilosophers.java line 98:
> 
>> 96: 
>> 97:         protected boolean tryPickForks(int fork1, int fork2) {
>> 98:             if (forks.getAndSet(fork1, 1) == 0) {
> 
> I don't believe these should be unconditional `getAndSet`, lest we want to have ABA problem here? I'd say it should be `forks.compareAndSet(fork1, 0, 1)`.

Good idea! Done.

> jcstress-samples/src/main/java/org/openjdk/jcstress/samples/concurrency/classic/ClassicProblem_01_DiningPhilosophers.java line 118:
> 
>> 116:     @State
>> 117:     public static class Arbitrator extends Base {
>> 118:         private final Semaphore waiter = new Semaphore(1);
> 
> Same here, not a `Semaphore`, but `Object`.

Good idea! Done.

> jcstress-samples/src/main/java/org/openjdk/jcstress/samples/concurrency/classic/ClassicProblem_01_DiningPhilosophers.java line 149:
> 
>> 147:                 waiter.release();
>> 148: 
>> 149:                 if(hasForks) {
> 
> Suggestion:
> 
>                 if (hasForks) {

Done.

> jcstress-samples/src/main/java/org/openjdk/jcstress/samples/concurrency/classic/ClassicProblem_01_DiningPhilosophers.java line 196:
> 
>> 194:                 diners.release();
>> 195: 
>> 196:                 if(hasForks) {
> 
> Suggestion:
> 
>                 if (hasForks) {

Done.

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

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