[crac] RFR: Introduce per-Priority Context with different policies [v4]

[Radim Vansa]

On Tue, 30 May 2023 16:14:10 GMT, Anton Kozlov <akozlov at openjdk.org> wrote:

>> A follow-up work for #60:
>> 
>> * Each priority now has a dedicated context, so contextes may provide different policies.
>> * the Global Context reverted from BlockingOrderedContext to OrderedContext, as that may have a huge impact on users. Probably we'll want to expose blocking/criticalUnorderd context along the global one, or at some point expose an implementation. But this is also out of scope of the PR.
>> * hierachy of the Context implementations are cleaned up a bit [2]
>> 
>> The JDKContext is now just a holder of ClaimedFDs, I'll address this in a follow-up that depends on this Context follow-up.
>> 
>> [1] https://github.com/openjdk/crac/pull/60#issuecomment-1545588281
>> [2] https://github.com/openjdk/crac/pull/60#discussion_r1185510445
>
> Anton Kozlov has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Update

src/java.base/share/classes/java/lang/ref/Cleaner.java line 225:

> 223: 
> 224:     /**
> 225:      * Register an object and object and also register the underlying Reference with a CRaC priority.

Typo

src/java.base/share/classes/javax/crac/Core.java line 40:

> 38:     }
> 39: 
> 40:     private static final Context<Resource> globalContext = new ContextWrapper(new OrderedContext<>());

Please use BlockingOrderedContext here as well, for symmetry.

src/java.base/share/classes/jdk/crac/Core.java line 124:

> 122: 
> 123:         try {
> 124:             jdk.internal.crac.Core.getJDKContext().beforeCheckpoint(null);

Why don't you register this as a resource? Besides, the way you did it does not work correctly if exceptions are thrown.

src/java.base/share/classes/jdk/crac/impl/OrderedContext.java line 38:

> 36:  * @param <R>
> 37:  */
> 38: public class OrderedContext<R extends Resource> extends AbstractContext<R> {

This class should not be used anywhere directly, and is not exposed to users => should be abstract.

src/java.base/share/classes/jdk/internal/crac/Core.java line 35:

> 33:     private static JDKContext jdkContext = new JDKContext();
> 34: 
> 35:     public static JDKContext getJDKContext() {

Please rename the method along with `JDKContext` rename.

src/java.base/share/classes/jdk/internal/crac/JDKContext.java line 46:

> 44: import java.util.WeakHashMap;
> 45: 
> 46: public class JDKContext implements JDKResource {

Since this is not a context anymore I believe that in the current form the class would deserve a rename (`ClaimedFileDescriptors`?)

test/jdk/jdk/crac/ContextOrderTest.java line 102:

> 100:     private static void testRegisterBlocks() throws Exception {
> 101:         var recorder = new LinkedList<String>();
> 102:         BlockingOrderedContext<Resource> blockingCtx = new BlockingOrderedContext();

We don't need separate contexts now that global one is blocking.

test/jdk/jdk/crac/ContextOrderTest.java line 147:

> 145:                         thread.interrupt();
> 146:                         thread.join(TimeUnit.NANOSECONDS.toMillis(deadline - System.nanoTime()));
> 147:                         System.out.println(thread.getState() + " " + thread.isAlive());

Looks like forgotten debug logging

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

PR Review Comment: https://git.openjdk.org/crac/pull/74#discussion_r1211119470
PR Review Comment: https://git.openjdk.org/crac/pull/74#discussion_r1211120888
PR Review Comment: https://git.openjdk.org/crac/pull/74#discussion_r1211123029
PR Review Comment: https://git.openjdk.org/crac/pull/74#discussion_r1211127722
PR Review Comment: https://git.openjdk.org/crac/pull/74#discussion_r1211135018
PR Review Comment: https://git.openjdk.org/crac/pull/74#discussion_r1211131166
PR Review Comment: https://git.openjdk.org/crac/pull/74#discussion_r1211138301
PR Review Comment: https://git.openjdk.org/crac/pull/74#discussion_r1211139415