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

Thu May 18 12:40:15 UTC 2023

On Wed, 17 May 2023 14:44:08 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. CALL_SITE now uses new CriticalUnorderedContext that runs beforeCheckpoint on concurrent registration, fixes [1]. Whether or not CALL_SITE needs to be registered to at all is an open question and out of scope of this PR.
> * 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

My main objection is allowing the checkpoint to proceed even if a resource in the `CriticalUnorderedContext` throws an exception.

However, when I started the review I was hoping that you'll be able to remove the special priority for call sites and broken encapsulation in `MethodHandleNatives`. You've made this change a explicitly out of scope, but without this and no extra test that would demonstrate a previously flawed behavior it is unclear what is the actual benefit this PR brings.

src/java.base/share/classes/jdk/crac/impl/BlockingOrderedContext.java line 20:

> 18:             // We won't cause IllegalStateException because this is not an unexpected state
> 19:             // from the point of CRaC - it probably tried to register some code before.
> 20:             throw new RuntimeException("Interrupted thread tried to block in registration of " + resource + " in " + this);

The use of `this` in the exception relies on naming the context and the `toString()` method for easy identification. Since you've removed these it will show only class type rather than Global Context/custom name/JDK resource priority.

src/java.base/share/classes/jdk/crac/impl/CriticalUnorderedContext.java line 54:

> 52:         synchronized (this) {
> 53:             ExceptionHolder<CheckpointException> e = concurrentCheckpointException;
> 54:             concurrentCheckpointException = new ExceptionHolder<>(CheckpointException::new);

Rather than replacing the instance, could we make `throwIfAny` reset its state before throwing? That way we won't get the same exception thrown twice by accident.

src/java.base/share/classes/jdk/crac/impl/CriticalUnorderedContext.java line 83:

> 81:             if (concurrentCheckpointException != null) {
> 82:                 try {
> 83:                     invokeBeforeCheckpoint(resource);

This context is blocking as well - since you hold the monitor on this object if the code tries to indirectly (via operation in another thread) register other resource, it will deadlock, and cannot be woken up via interrupt.
If the only usage is call sites we know that there won't be a recursive registration, so this not such a big flaw, though.

src/java.base/share/classes/jdk/crac/impl/CriticalUnorderedContext.java line 85:

> 83:                     invokeBeforeCheckpoint(resource);
> 84:                 } catch (Exception e) {
> 85:                     concurrentCheckpointException.handle(e);

I **really** dislike the fact that the exception is reported only during restore, which may never happen. The checkpoint should be marked for failure in here.

src/java.base/share/classes/jdk/crac/impl/ExceptionHolder.java line 37:

> 35:         E exception = get();
> 36:         if (exception.getClass() == e.getClass()) {
> 37:             for (Throwable t : e.getSuppressed()) {

We're losing the message and stack trace here. Previously, if the message was present it was added to the suppressed list as well.
If you want to create aggregate-only exceptions (only suppressed list would be relevant) these should be declared as final, with only one no-arg constructor and stack trace collection disabled - but that would have some problems as discussed in https://github.com/openjdk/crac/pull/64/files#r1190679470

src/java.base/share/classes/jdk/internal/crac/JDKResource.java line 31:

> 29: import jdk.crac.Resource;
> 30: 
> 31: public interface JDKResource extends Resource {

Could we drop the interface completely?

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

> 65:         var recorder = new LinkedList<String>();
> 66:         getGlobalContext().register(new MockResource(recorder, null, "regular1"));
> 67:         JDKResource resource2 = new MockResource(recorder, NORMAL, "jdk-normal");

Looks like the local vars are not used?

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

> 104: 
> 105:         // blocks register into the same OrderedContext
> 106:         Context<Resource> context = new BlockingOrderedContext<>();

Why does this create a children context rather than using global one directly?

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

> 254: 
> 255:             if (priority != null) {
> 256:                 priority.getContext().register(this);

Registering to global context explicitly and to local one implicitly is confusing; if the mock does not require the `priority` field just remove the constructor arg and register it directly as well.

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

> 286:         private CreatingResource(List<String> recorder, Priority priority, String id, Priority childPriority) {
> 287:             super(recorder, priority, id);
> 288:             this.childContext = (Context<R>) childPriority.getContext(); // XXX

`// XXX`

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

Changes requested by rvansa at github.com (no known OpenJDK username).

PR Review: https://git.openjdk.org/crac/pull/74#pullrequestreview-1432550985
PR Review Comment: https://git.openjdk.org/crac/pull/74#discussion_r1197759128
PR Review Comment: https://git.openjdk.org/crac/pull/74#discussion_r1197722674
PR Review Comment: https://git.openjdk.org/crac/pull/74#discussion_r1197740085
PR Review Comment: https://git.openjdk.org/crac/pull/74#discussion_r1197730194
PR Review Comment: https://git.openjdk.org/crac/pull/74#discussion_r1197726046
PR Review Comment: https://git.openjdk.org/crac/pull/74#discussion_r1197762902
PR Review Comment: https://git.openjdk.org/crac/pull/74#discussion_r1197751634
PR Review Comment: https://git.openjdk.org/crac/pull/74#discussion_r1197747285
PR Review Comment: https://git.openjdk.org/crac/pull/74#discussion_r1197756264
PR Review Comment: https://git.openjdk.org/crac/pull/74#discussion_r1197748743