On Tue, 15 Nov 2022 17:36:16 GMT, Andrew Haley <a...@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
