Re: RFR: JDK-8286666: JEP 429: Implementation of Scoped Values (Incubator) [v10]

[Paul Sandoz]

On Wed, 16 Nov 2022 16:55:24 GMT, Andrew Haley <a...@openjdk.org> wrote:

>> JEP 429 implementation.
>
> Andrew Haley has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Javadoc changes.
>  - ProblemList.txt cleanup

src/java.base/share/classes/java/lang/Thread.java line 744:

> 742: 
> 743:         // special value to mean a new thread
> 744:         this.scopedValueBindings = Thread.class;

Perhaps:

    static final Object NEW_THREAD_BINDINGS = Thread.class;
    ...
    this.scopedValueBindings = NEW_THREAD_BINDINGS;

?

src/java.base/share/classes/java/lang/Thread.java line 1614:

> 1612:     }
> 1613: 
> 1614:     @Hidden

Should we document that the name and signature are special (same of other 
relevant methods not already documented).

src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/ScopedValue.java
 line 49:

> 47:  *
> 48:  * <p> {@code ScopedValue} defines the {@link #where(ScopedValue, Object, 
> Runnable)}
> 49:  * method to set the value of a {@code ScopedValue} for the bouned period 
> of execution by

Suggestion:

 * method to set the value of a {@code ScopedValue} for the bounded period of 
execution by

src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/ScopedValue.java
 line 241:

> 239:     }
> 240: 
> 241:     static final class EmptySnapshot extends Snapshot {

We could make `Snapshot` final have a static final field?

   static final Snapshot EMPTY_SNAPSHOT = new Snapshot();

src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/ScopedValue.java
 line 391:

> 389:          * JVM_FindScopedValueBindings().
> 390:          */
> 391:         private <R> R runWith(Snapshot newSnapshot, Callable<R> op) 
> throws Exception {

Missing `@Hidden` and `@ForceInline` ? like with the other `runWith` method 
accepting `Runnable`?
(I was gonna suggest changing the name to `callWith`, but then reread the docs 
and VM code. Its convenient to have the same names.)

src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/ScopedValue.java
 line 672:

> 670:            return EmptySnapshot.getInstance();
> 671:         }
> 672:         if (bindings == null) {

Suggestion:

        if (bindings == NEW_THREAD_BINDINGS) {
            // This must be a new thread
           return Snapshot.EMPTY_SNAPSHOT;
        } else if (bindings == null) {

src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/ScopedValue.java
 line 720:

> 718:     // is invoked, we record the result of the lookup in this per-thread 
> cache
> 719:     // for fast access in future.
> 720:     private static class Cache {

Make the class final and remove the qualifier on all the methods.

src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/ScopedValue.java
 line 824:

> 822:         }
> 823: 
> 824:         public static void invalidate() {

Is this method used?

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

PR: https://git.openjdk.org/jdk/pull/10952