Re: RFR: JDK-8286666: JEP 429: Implementation of Scoped Values (Incubator) [v3]
> JEP 429 implementation. Andrew Haley has updated the pull request incrementally with one additional commit since the last revision: Reviewer feedback - Changes: - all: https://git.openjdk.org/jdk/pull/10952/files - new: https://git.openjdk.org/jdk/pull/10952/files/4bd44d66..442a04ef Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10952=02 - incr: https://webrevs.openjdk.org/?repo=jdk=10952=01-02 Stats: 2 lines in 1 file changed: 0 ins; 2 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/10952.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10952/head:pull/10952 PR: https://git.openjdk.org/jdk/pull/10952
Re: RFR: JDK-8286666: JEP 429: Implementation of Scoped Values (Incubator) [v2]
On Tue, 15 Nov 2022 14:15:26 GMT, Andrew Haley wrote: >> src/java.base/share/classes/java/lang/VirtualThread.java line 318: >> >>> 316: } >>> 317: } >>> 318: @Hidden >> >> Can we rename this to runWith(Runnable, Object) in both Thread and >> VirtualThread to keep the naming consistent if we can? > > OK. I do want to keep the name of this method consistent throughout. There > are version for `Runnable` and `Callable` and it makes the runtime code > cleaner if they have the same name. The other thing here is that there two calls to reachabilityFence, it shouldn't need both. - PR: https://git.openjdk.org/jdk/pull/10952
Re: RFR: JDK-8286666: JEP 429: Implementation of Scoped Values (Incubator) [v2]
> JEP 429 implementation. Andrew Haley has updated the pull request incrementally with two additional commits since the last revision: - Update test - Reviewer feedback - Changes: - all: https://git.openjdk.org/jdk/pull/10952/files - new: https://git.openjdk.org/jdk/pull/10952/files/0d72ca2f..4bd44d66 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10952=01 - incr: https://webrevs.openjdk.org/?repo=jdk=10952=00-01 Stats: 6 lines in 2 files changed: 0 ins; 3 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/10952.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10952/head:pull/10952 PR: https://git.openjdk.org/jdk/pull/10952
Re: RFR: JDK-8286666: JEP 429: Implementation of Scoped Values (Incubator)
On Wed, 2 Nov 2022 16:23:34 GMT, Andrew Haley wrote:
> JEP 429 implementation.
src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/ScopedValue.java
line 475:
> 473: // ??? Do we want to search cache for this? In most cases we
> don't expect
> 474: // this {@link ScopedValue} to be bound, so it's not worth it.
> But I may
> 475: // be wrong about that.
We should make it switchable by a runtime parameter. We should optionally cache
failures.
Add a microbenchmark for that.
-
PR: https://git.openjdk.org/jdk/pull/10952
Re: RFR: JDK-8286666: JEP 429: Implementation of Scoped Values (Incubator)
On Mon, 14 Nov 2022 17:34:31 GMT, Alan Bateman wrote: >> JEP 429 implementation. > > src/java.base/share/classes/java/lang/VirtualThread.java line 318: > >> 316: } >> 317: } >> 318: @Hidden > > Can we rename this to runWith(Runnable, Object) in both Thread and > VirtualThread to keep the naming consistent if we can? OK. I do want to keep the name of this method consistent throughout. There are version for `Runnable` and `Callable` and it makes the runtime code cleaner if they have the same name. - PR: https://git.openjdk.org/jdk/pull/10952
Re: RFR: JDK-8286666: JEP 429: Implementation of Scoped Values (Incubator)
On Fri, 4 Nov 2022 09:53:39 GMT, Andrew Haley wrote:
>> 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.
I hope it doesn't matter. There is an example in the reachabilityFence
javadocs where it does not use finally. For it to matter, I think the compiler
would need to inline through run() and prove that it can throw an exception,
but I don't think that's how the JIT compilers currently implement
reachabilityFence. I suppose a finally shouldn't hurt, however.
-
PR: https://git.openjdk.org/jdk/pull/10952
Re: RFR: JDK-8286666: JEP 429: Implementation of Scoped Values (Incubator)
On Thu, 3 Nov 2022 11:50:17 GMT, ExE Boss 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
Re: RFR: JDK-8286666: JEP 429: Implementation of Scoped Values (Incubator)
On Wed, 2 Nov 2022 16:23:34 GMT, Andrew Haley 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);
}
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;
-
PR: https://git.openjdk.org/jdk/pull/10952
Re: RFR: JDK-8286666: JEP 429: Implementation of Scoped Values (Incubator)
On Wed, 2 Nov 2022 16:23:34 GMT, Andrew Haley wrote:
> JEP 429 implementation.
src/hotspot/share/prims/jvm.cpp line 4072:
> 4070: */
> 4071: JVM_ENTRY(void, JVM_EnsureMaterializedForStackWalk_func(JNIEnv* env,
> jobject vthread, jobject value))
> 4072: //asm("nop");
The asm("nop") was commented out to get the build working. Its inserted by the
compiler now so I assume the commented now asm can be removed.
src/java.base/share/classes/java/lang/VirtualThread.java line 318:
> 316: }
> 317: }
> 318: @Hidden
Can we rename this to runWith(Runnable, Object) in both Thread and
VirtualThread to keep the naming consistent if we can?
-
PR: https://git.openjdk.org/jdk/pull/10952
Re: RFR: JDK-8286666: JEP 429: Implementation of Scoped Values (Incubator)
On Thu, 10 Nov 2022 17:42:38 GMT, Andrew Haley wrote:
>> src/hotspot/share/prims/jvm.cpp line 1410:
>>
>>> 1408: loc = 3;
>>> 1409: } else if (method == resolver.thread_run_method) {
>>> 1410: loc = 2;
>>
>> This depends on how javac numbers locals, right? It seems a bit fragile.
>> This is one of the reasons why doPrivileged uses the helper method
>> executePrivileged, so the locals are arguments, giving them predictable
>> offsets.
>
> Ah, good point. I'll have a look at doing that.
Done.
-
PR: https://git.openjdk.org/jdk/pull/10952
Re: RFR: JDK-8286666: JEP 429: Implementation of Scoped Values (Incubator)
On Fri, 4 Nov 2022 23:17:32 GMT, Dean Long wrote:
>> JEP 429 implementation.
>
> src/hotspot/share/prims/jvm.cpp line 1410:
>
>> 1408: loc = 3;
>> 1409: } else if (method == resolver.thread_run_method) {
>> 1410: loc = 2;
>
> This depends on how javac numbers locals, right? It seems a bit fragile.
> This is one of the reasons why doPrivileged uses the helper method
> executePrivileged, so the locals are arguments, giving them predictable
> offsets.
Ah, good point. I'll have a look at doing that.
-
PR: https://git.openjdk.org/jdk/pull/10952
Re: RFR: JDK-8286666: JEP 429: Implementation of Scoped Values (Incubator)
On Wed, 2 Nov 2022 16:23:34 GMT, Andrew Haley wrote:
> JEP 429 implementation.
src/hotspot/share/prims/jvm.cpp line 1410:
> 1408: loc = 3;
> 1409: } else if (method == resolver.thread_run_method) {
> 1410: loc = 2;
This depends on how javac numbers locals, right? It seems a bit fragile. This
is one of the reasons why doPrivileged uses the helper method
executePrivileged, so the locals are arguments, giving them predictable offsets.
src/java.base/share/classes/jdk/internal/vm/ScopedValueContainer.java line 53:
> 51: /**
> 52: * Returns the "latest" ScopedValueContainer for the current Thread.
> This may be on
> 53: * the current thread's scope task or ma require walking up the tree
> to find it.
Suggestion:
* the current thread's scope task or may require walking up the tree to
find it.
-
PR: https://git.openjdk.org/jdk/pull/10952
