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

[Maurizio Cimadamore]

On Tue, 15 Nov 2022 17:36:16 GMT, Andrew Haley <aph at openjdk.org> wrote:

>> JEP 429 implementation.
>
> Andrew Haley has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix failing serviceability tests

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

> 41: /**
> 42:  * A value that is set once and is then available for reading for a bounded period of
> 43:  * execution by a thread. A {@code ScopedValue} allows for safely and efficiently sharing

by a thread, or by one or more threads? (when inherited)

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

> 158:  * record.
> 159:  *
> 160:  * <p>For this incubator release, we have provided some system properties

Maybe it would be better to frame this as "The reference implementation provides some system properties". The term "reference implementation" is used elsewhere to define JDK specific mechanisms that might, or might not carry across to other JVM/Java SE API implementations.

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

> 170:  * must be an integer power of 2.
> 171:  *
> 172:  * <p>For example, you could use {@code -Djdk.incubator.concurrent.ScopedValue.cacheSize=8}.

I would also avoid "you" and "we" in the javadoc. While javadoc is not formal, we often use locutions such as "clients can use/do XYZ".

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

> 178:  * thread preserves its scoped-value cache when blocked. Like {@code
> 179:  * ScopedValue.cacheSize}, this is a space versus speed trade-off: if
> 180:  * you have a great many virtual threads that are blocked most of the

"in situations where many virtual threads are blocked most of the time, ..."

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

> 183:  * would have to be regenerated after a blocking operation.
> 184:  *
> 185:  * @param <T> the type of the value

Suggestion:

 * @param <T> the type of the scoped value

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

> 352: 
> 353:         /**
> 354:          * Calls a value returning operation with each scoped value in this mapping bound

Suggestion:

         * Calls a value-returning operation with each scoped value in this mapping bound

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

> 458:      * }
> 459:      *
> 460:      * @param key the ScopedValue key

should use `@code` or `@link`

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

> 461:      * @param value the value, can be {@code null}
> 462:      * @param <T> the type of the value
> 463:      * @return a new Carrier with a single mapping

same here

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

> 468: 
> 469:     /**
> 470:      * Calls a value returning operation with a {@code ScopedValue} bound to a value

Suggestion:

     * Calls a value returning-operation with a {@code ScopedValue} bound to a value

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

> 488:      * }
> 489:      *
> 490:      * @param key the ScopedValue

Again, missing `@code` - please check all methods

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

> 611:      * @return the value of the scoped value if bound, otherwise {@code other}
> 612:      */
> 613:     public T orElse(T other) {

>From an API perspective, wouldn't return `Optional` a more consistent choice? `Optional` has all methods we need to deal with this kind of stuff...

src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/StructuredTaskScope.java line 229:

> 227:  *   <li> Inheritance of {@linkplain ScopedValue scoped values} across threads.
> 228:  *   <li> Confinement checks. The phrase "threads contained in the task scope" in method
> 229:  *   descriptions means threads started in the task scope or descendant scopes.

sadly, the term "descendant scopes" is not defined elsewhere in this javadoc

src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/StructuredTaskScope.java line 233:

> 231:  *
> 232:  * <p> The following example demonstrates the inheritance of a scoped value. A scoped
> 233:  * value {@code USERNAME} is bound to the value "duke". A StructuredTaskScope is created

Missing `@code` or `@link` for `StructuredTaskScope`

src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/StructuredTaskScope.java line 237:

> 235:  * The thread inherits the scoped value <em>bindings</em> captured when creating the
> 236:  * task scope. The code in {@code childTask} uses the value of the scoped value and so
> 237:  * reads the value "duke".

Suggestion:

 * reads the value {@code "duke"}.

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

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