On Thu, 3 Nov 2022 11:50:17 GMT, ExE Boss <d...@openjdk.org> wrote:
>> JEP 429 implementation.
>
> src/java.base/share/classes/java/lang/Thread.java line 1610:
>
>> 1608: ensureMaterializedForStackWalk(bindings);
>> 1609: task.run();
>> 1610: Reference.reachabilityFence(bindings);
>
> This should probably be in a `try`‑`finally` block:
> Suggestion:
>
> try {
> task.run();
> } finally {
> Reference.reachabilityFence(bindings);
> }
I wonder. The pattern I'm using here is based on
`AccessController.executePrivileged`, which doesn't have the `finally` clause.
Perhaps I should add one here anyway.
> src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/ScopedValue.java
> line 481:
>
>> 479: }
>> 480: */
>> 481: return findBinding() != Snapshot.NIL;
>
> This should probably call `Cache.put(this, value)` when `findBinding()` isn’t
> `Snapshot.NIL`, since it’s likely that `isBound()` will most commonly be used
> in the form of:
>
> if (SCOPED_VALUE.isBound()) {
> final var value = SCOPED_VALUE.get();
> // do something with `value`
> }
>
>
> --------------------------------------------------------------------------------
>
> Suggestion:
>
> var value = findBinding();
> if (value == Snapshot.NIL) {
> return false;
> }
> Cache.put(this, value);
> return true;
Probably so, yes. I'll have a look at that along with caching failure.
-------------
PR: https://git.openjdk.org/jdk/pull/10952
