Re: RFR: 8284199: Implementation of Structured Concurrency (Incubator) [v5]

2022-06-03 Thread Joe Darcy
On Thu, 2 Jun 2022 11:25:36 GMT, Alan Bateman  wrote:

>> This is the implementation of JEP 428: Structured Concurrency (Incubator).
>> 
>> This is a non-final API that provides a gentle on-ramp to structure a task 
>> as a family of concurrent subtasks, and to coordinate the subtasks as a unit.
>
> Alan Bateman has updated the pull request with a new target base due to a 
> merge or a rebase. The incremental webrev excludes the unrelated changes 
> brought in by the merge/rebase. The pull request contains 12 additional 
> commits since the last revision:
> 
>  - Add test directory to jdk_other so tests run in tier2
>  - Merge
>  - Fix typo in javadoc
>  - Merge
>  - Javadoc updates, add to jdk_loom test group
>  - Add statement to close about thread termination
>  - Use {@code ...}, replace task->subtask, fix typos, add jls ref
>  - Merge
>  - @ignore StructuredThreadDumpTest until test infra in place
>  - Refresh
>  - ... and 2 more: 
> https://git.openjdk.java.net/jdk/compare/3c34c506...7f48656e

Marked as reviewed by darcy (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/8787


Re: RFR: 8284199: Implementation of Structured Concurrency (Incubator) [v5]

2022-06-02 Thread Alan Bateman
> This is the implementation of JEP 428: Structured Concurrency (Incubator).
> 
> This is a non-final API that provides a gentle on-ramp to structure a task as 
> a family of concurrent subtasks, and to coordinate the subtasks as a unit.

Alan Bateman has updated the pull request with a new target base due to a merge 
or a rebase. The incremental webrev excludes the unrelated changes brought in 
by the merge/rebase. The pull request contains 12 additional commits since the 
last revision:

 - Add test directory to jdk_other so tests run in tier2
 - Merge
 - Fix typo in javadoc
 - Merge
 - Javadoc updates, add to jdk_loom test group
 - Add statement to close about thread termination
 - Use {@code ...}, replace task->subtask, fix typos, add jls ref
 - Merge
 - @ignore StructuredThreadDumpTest until test infra in place
 - Refresh
 - ... and 2 more: https://git.openjdk.java.net/jdk/compare/d40ed463...7f48656e

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8787/files
  - new: https://git.openjdk.java.net/jdk/pull/8787/files/d11b24bc..7f48656e

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=8787=04
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8787=03-04

  Stats: 46596 lines in 447 files changed: 23349 ins; 17945 del; 5302 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8787.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8787/head:pull/8787

PR: https://git.openjdk.java.net/jdk/pull/8787


Re: RFR: 8284199: Implementation of Structured Concurrency (Incubator) [v4]

2022-05-30 Thread Alan Bateman
> This is the implementation of JEP 428: Structured Concurrency (Incubator).
> 
> This is a non-final API that provides a gentle on-ramp to structure a task as 
> a family of concurrent subtasks, and to coordinate the subtasks as a unit.

Alan Bateman has updated the pull request with a new target base due to a merge 
or a rebase. The incremental webrev excludes the unrelated changes brought in 
by the merge/rebase. The pull request contains 10 additional commits since the 
last revision:

 - Fix typo in javadoc
 - Merge
 - Javadoc updates, add to jdk_loom test group
 - Add statement to close about thread termination
 - Use {@code ...}, replace task->subtask, fix typos, add jls ref
 - Merge
 - @ignore StructuredThreadDumpTest until test infra in place
 - Refresh
 - Merge
 - Initial commit

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8787/files
  - new: https://git.openjdk.java.net/jdk/pull/8787/files/4fc454a9..d11b24bc

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=8787=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8787=02-03

  Stats: 33428 lines in 507 files changed: 6180 ins; 25559 del; 1689 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8787.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8787/head:pull/8787

PR: https://git.openjdk.java.net/jdk/pull/8787


Re: RFR: 8284199: Implementation of Structured Concurrency (Incubator) [v3]

2022-05-26 Thread Joe Darcy
On Tue, 24 May 2022 15:15:43 GMT, Alan Bateman  wrote:

>> This is the implementation of JEP 428: Structured Concurrency (Incubator).
>> 
>> This is a non-final API that provides a gentle on-ramp to structure a task 
>> as a family of concurrent subtasks, and to coordinate the subtasks as a unit.
>
> Alan Bateman has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Add statement to close about thread termination

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

> 666: sb.append('/');
> 667: }
> 668: String id = getClass().getName() + "@" + 
> System.identityHashCode(this);

Can use Objects.toIdentityString() instead.

-

PR: https://git.openjdk.java.net/jdk/pull/8787


Re: RFR: 8284199: Implementation of Structured Concurrency (Incubator) [v3]

2022-05-25 Thread Maurizio Cimadamore
On Tue, 24 May 2022 15:15:43 GMT, Alan Bateman  wrote:

>> This is the implementation of JEP 428: Structured Concurrency (Incubator).
>> 
>> This is a non-final API that provides a gentle on-ramp to structure a task 
>> as a family of concurrent subtasks, and to coordinate the subtasks as a unit.
>
> Alan Bateman has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Add statement to close about thread termination

Looks good!

-

Marked as reviewed by mcimadamore (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/8787


Re: RFR: 8284199: Implementation of Structured Concurrency (Incubator) [v3]

2022-05-24 Thread Alan Bateman
> This is the implementation of JEP 428: Structured Concurrency (Incubator).
> 
> This is a non-final API that provides a gentle on-ramp to structure a task as 
> a family of concurrent subtasks, and to coordinate the subtasks as a unit.

Alan Bateman has updated the pull request incrementally with one additional 
commit since the last revision:

  Add statement to close about thread termination

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8787/files
  - new: https://git.openjdk.java.net/jdk/pull/8787/files/c72f0330..4fc454a9

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=8787=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8787=01-02

  Stats: 12 lines in 3 files changed: 5 ins; 0 del; 7 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8787.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8787/head:pull/8787

PR: https://git.openjdk.java.net/jdk/pull/8787


Re: RFR: 8284199: Implementation of Structured Concurrency (Incubator) [v2]

2022-05-24 Thread Alan Bateman
On Tue, 24 May 2022 10:52:07 GMT, Maurizio Cimadamore  
wrote:

>> Alan Bateman has updated the pull request with a new target base due to a 
>> merge or a rebase. The incremental webrev excludes the unrelated changes 
>> brought in by the merge/rebase. The pull request contains six additional 
>> commits since the last revision:
>> 
>>  - Use {@code ...}, replace task->subtask, fix typos, add jls ref
>>  - Merge
>>  - @ignore StructuredThreadDumpTest until test infra in place
>>  - Refresh
>>  - Merge
>>  - Initial commit
>
> src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/StructuredTaskScope.java
>  line 214:
> 
>> 212:  * Tree structure
>> 213:  *
>> 214:  * StructuredTaskScopes form a tree where parent-child relations are 
>> established
> 
> Should we mention what happens in the owner thread completes its execution 
> and the scope's `close` method has not been called? I think that, as 
> discussed offline, the fact that the thread will attempt to close any nested 
> scopes when terminating is an important aspect of this API.

The close method might be the right place for this. It has to specify that an 
attempt to close out of order will close the nested scopes and terminating 
without close is much the same. I'll see what I can do, thanks for this 
suggestion.

-

PR: https://git.openjdk.java.net/jdk/pull/8787


Re: RFR: 8284199: Implementation of Structured Concurrency (Incubator) [v2]

2022-05-24 Thread Alan Bateman
On Tue, 24 May 2022 10:48:02 GMT, Maurizio Cimadamore  
wrote:

>> More generally, I see that you used `{@code ... }` in a lot of places where 
>> `{@link ... }` could also be used. In some of those places (like this one) 
>> where there is a clear cross-reference, I think `@link` could be 
>> preferrable. The only case where `@code` is fine is when referring to the 
>> name of the class itself (e.g. `{@code StructuredTaskScope}`). But of course 
>> this is subjective.
>
> Also, note the typo `the join is invoked`. Either `the` is dropped, or 
> `method` is added. I've seen more than one occurrence of this.

It should be "before join is invoked". It doesn't use a link here because it 
already links to join and joinUntil in the previous sentence.

-

PR: https://git.openjdk.java.net/jdk/pull/8787


Re: RFR: 8284199: Implementation of Structured Concurrency (Incubator) [v2]

2022-05-24 Thread Maurizio Cimadamore
On Tue, 24 May 2022 10:47:15 GMT, Maurizio Cimadamore  
wrote:

>> src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/StructuredTaskScope.java
>>  line 1110:
>> 
>>> 1108:  * invoked {@link #join() join} (or {@link 
>>> #joinUntil(Instant) joinUntil}).
>>> 1109:  * The behavior of this method is unspecified when invoking 
>>> this method before
>>> 1110:  * the {@code join} is invoked.
>> 
>> Suggestion:
>> 
>>  * {@link #join} is invoked.
>
> More generally, I see that you used `{@code ... }` in a lot of places where 
> `{@link ... }` could also be used. In some of those places (like this one) 
> where there is a clear cross-reference, I think `@link` could be preferrable. 
> The only case where `@code` is fine is when referring to the name of the 
> class itself (e.g. `{@code StructuredTaskScope}`). But of course this is 
> subjective.

Also, note the typo `the join is invoked`. Either `the` is dropped, or `method` 
is added. I've seen more than one occurrence of this.

-

PR: https://git.openjdk.java.net/jdk/pull/8787


Re: RFR: 8284199: Implementation of Structured Concurrency (Incubator) [v2]

2022-05-24 Thread Maurizio Cimadamore
On Tue, 24 May 2022 10:41:59 GMT, Alan Bateman  wrote:

>> This is the implementation of JEP 428: Structured Concurrency (Incubator).
>> 
>> This is a non-final API that provides a gentle on-ramp to structure a task 
>> as a family of concurrent subtasks, and to coordinate the subtasks as a unit.
>
> Alan Bateman has updated the pull request with a new target base due to a 
> merge or a rebase. The incremental webrev excludes the unrelated changes 
> brought in by the merge/rebase. The pull request contains six additional 
> commits since the last revision:
> 
>  - Use {@code ...}, replace task->subtask, fix typos, add jls ref
>  - Merge
>  - @ignore StructuredThreadDumpTest until test infra in place
>  - Refresh
>  - Merge
>  - Initial commit

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

> 212:  * Tree structure
> 213:  *
> 214:  * StructuredTaskScopes form a tree where parent-child relations are 
> established

Should we mention what happens in the owner thread completes its execution and 
the scope's `close` method has not been called? I think that, as discussed 
offline, the fact that the thread will attempt to close any nested scopes when 
terminating is an important aspect of this API.

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

> 1108:  * invoked {@link #join() join} (or {@link #joinUntil(Instant) 
> joinUntil}).
> 1109:  * The behavior of this method is unspecified when invoking 
> this method before
> 1110:  * the {@code join} is invoked.

Suggestion:

 * {@link #join} is invoked.

test/jdk/jdk/incubator/concurrent/StructuredTaskScope/StructuredThreadDumpTest.java
 line 200:

> 198: }
> 199: 
> 200: }

Watch out for the newline here

-

PR: https://git.openjdk.java.net/jdk/pull/8787


Re: RFR: 8284199: Implementation of Structured Concurrency (Incubator) [v2]

2022-05-24 Thread Maurizio Cimadamore
On Tue, 24 May 2022 10:44:39 GMT, Maurizio Cimadamore  
wrote:

>> Alan Bateman has updated the pull request with a new target base due to a 
>> merge or a rebase. The incremental webrev excludes the unrelated changes 
>> brought in by the merge/rebase. The pull request contains six additional 
>> commits since the last revision:
>> 
>>  - Use {@code ...}, replace task->subtask, fix typos, add jls ref
>>  - Merge
>>  - @ignore StructuredThreadDumpTest until test infra in place
>>  - Refresh
>>  - Merge
>>  - Initial commit
>
> src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/StructuredTaskScope.java
>  line 1110:
> 
>> 1108:  * invoked {@link #join() join} (or {@link #joinUntil(Instant) 
>> joinUntil}).
>> 1109:  * The behavior of this method is unspecified when invoking 
>> this method before
>> 1110:  * the {@code join} is invoked.
> 
> Suggestion:
> 
>  * {@link #join} is invoked.

More generally, I see that you used `{@code ... }` in a lot of places where 
`{@link ... }` could also be used. In some of those places (like this one) 
where there is a clear cross-reference, I think `@link` could be preferrable. 
The only case where `@code` is fine is when referring to the name of the class 
itself (e.g. `{@code StructuredTaskScope}`). But of course this is subjective.

-

PR: https://git.openjdk.java.net/jdk/pull/8787


Re: RFR: 8284199: Implementation of Structured Concurrency (Incubator) [v2]

2022-05-24 Thread Alan Bateman
> This is the implementation of JEP 428: Structured Concurrency (Incubator).
> 
> This is a non-final API that provides a gentle on-ramp to structure a task as 
> a family of concurrent subtasks, and to coordinate the subtasks as a unit.

Alan Bateman has updated the pull request with a new target base due to a merge 
or a rebase. The incremental webrev excludes the unrelated changes brought in 
by the merge/rebase. The pull request contains six additional commits since the 
last revision:

 - Use {@code ...}, replace task->subtask, fix typos, add jls ref
 - Merge
 - @ignore StructuredThreadDumpTest until test infra in place
 - Refresh
 - Merge
 - Initial commit

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8787/files
  - new: https://git.openjdk.java.net/jdk/pull/8787/files/6a9553b9..c72f0330

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=8787=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8787=00-01

  Stats: 6142 lines in 192 files changed: 3679 ins; 1909 del; 554 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8787.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8787/head:pull/8787

PR: https://git.openjdk.java.net/jdk/pull/8787


Re: RFR: 8284199: Implementation of Structured Concurrency (Incubator)

2022-05-24 Thread Alan Bateman
On Tue, 24 May 2022 04:18:44 GMT, Joe Darcy  wrote:

>> This is the implementation of JEP 428: Structured Concurrency (Incubator).
>> 
>> This is a non-final API that provides a gentle on-ramp to structure a task 
>> as a family of concurrent subtasks, and to coordinate the subtasks as a unit.
>
> src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/StructuredTaskScope.java
>  line 237:
> 
>> 235:  * the task result is retrieved via its {@code Future}, or 
>> happen-before any actions
>> 236:  * taken in a thread after {@linkplain #join() joining} of the task 
>> scope.
>> 237:  *
> 
> Would a @-jls reference to the appropriate section of the memory model 
> chapter help here?

Yes, it can reference JLS 17.4.5.

-

PR: https://git.openjdk.java.net/jdk/pull/8787


Re: RFR: 8284199: Implementation of Structured Concurrency (Incubator)

2022-05-24 Thread Alan Bateman
On Mon, 23 May 2022 21:09:24 GMT, Maurizio Cimadamore  
wrote:

>> This is the implementation of JEP 428: Structured Concurrency (Incubator).
>> 
>> This is a non-final API that provides a gentle on-ramp to structure a task 
>> as a family of concurrent subtasks, and to coordinate the subtasks as a unit.
>
> src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/StructuredTaskScope.java
>  line 88:
> 
>> 86:  * {@code join} method after forking.
>> 87:  *
>> 88:  *  StructuredTaskScope defines the {@link #shutdown() shutdown} 
>> method to shut down a
> 
> This sentence, because of the place where it appears, is a bit confusing. So 
> far we only know about the fact that a scope has an owner thread. So it seems 
> odd that shutdown could be called _while_ the owner thread is waiting on a 
> `join`. Of course, then you read what's next, and you discover that: (a) 
> shutdown might be called by a custom scope subclass and that (b) shutdown is 
> confined to the threads contained in this task scope - but this definition is 
> only given much later.

I see your point. The intention is to introduce all the public methods before 
introducing the subclasses or policies. I think I can adjust this sentence to 
make it clear that a subtask may call shutdown while the owner is waiting in 
join.

> src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/StructuredTaskScope.java
>  line 353:
> 
>> 351:  *
>> 352:  *  The {@code handleComplete} method should be thread safe. It 
>> may be
>> 353:  * invoked by several threads at around the same.
> 
> Something is missing? E.g. "at around the same TIME" ? (I'd suggest just 
> using "concurrently")

Thanks, it was supposed to say "around the same time" but saying "concurrently" 
would be better.

> src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/StructuredTaskScope.java
>  line 376:
> 
>> 374:  *
>> 375:  *  If this task scope is {@linkplain #shutdown() shutdown} (or 
>> in the process
>> 376:  * of shutting down) then {@code fork} returns a Future 
>> representing a {@link
> 
> Future in plaintext?

Yes, Daniel also pointed this point that there are a few uses of 
"StructuredTaskScope" that should also use {@code ...}.

-

PR: https://git.openjdk.java.net/jdk/pull/8787


Re: RFR: 8284199: Implementation of Structured Concurrency (Incubator)

2022-05-24 Thread Alan Bateman
On Mon, 23 May 2022 13:11:29 GMT, Daniel Fuchs  wrote:

>> This is the implementation of JEP 428: Structured Concurrency (Incubator).
>> 
>> This is a non-final API that provides a gentle on-ramp to structure a task 
>> as a family of concurrent subtasks, and to coordinate the subtasks as a unit.
>
> src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/StructuredTaskScope.java
>  line 54:
> 
>> 52: 
>> 53: /**
>> 54:  * A basic API for structured concurrency. StructuredTaskScope 
>> supports cases
> 
> Should StructuredTaskScope in this class-level API doc comment be surrounded 
> by `{@code }` to appear in code font?

Okay, there are quite a few of these so I may have to adjust some of the lines 
to avoid too many inconsistent line lengths.

-

PR: https://git.openjdk.java.net/jdk/pull/8787


Re: RFR: 8284199: Implementation of Structured Concurrency (Incubator)

2022-05-23 Thread Joe Darcy
On Thu, 19 May 2022 13:05:54 GMT, Alan Bateman  wrote:

> This is the implementation of JEP 428: Structured Concurrency (Incubator).
> 
> This is a non-final API that provides a gentle on-ramp to structure a task as 
> a family of concurrent subtasks, and to coordinate the subtasks as a unit.

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

> 235:  * the task result is retrieved via its {@code Future}, or 
> happen-before any actions
> 236:  * taken in a thread after {@linkplain #join() joining} of the task 
> scope.
> 237:  *

Would a @-jls reference to the appropriate section of the memory model chapter 
help here?

-

PR: https://git.openjdk.java.net/jdk/pull/8787


Re: RFR: 8284199: Implementation of Structured Concurrency (Incubator)

2022-05-23 Thread Maurizio Cimadamore
On Thu, 19 May 2022 13:05:54 GMT, Alan Bateman  wrote:

> This is the implementation of JEP 428: Structured Concurrency (Incubator).
> 
> This is a non-final API that provides a gentle on-ramp to structure a task as 
> a family of concurrent subtasks, and to coordinate the subtasks as a unit.

Marked as reviewed by mcimadamore (Reviewer).

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

> 86:  * {@code join} method after forking.
> 87:  *
> 88:  *  StructuredTaskScope defines the {@link #shutdown() shutdown} 
> method to shut down a

This sentence, because of the place where it appears, is a bit confusing. So 
far we only know about the fact that a scope has an owner thread. So it seems 
odd that shutdown could be called _while_ the owner thread is waiting on a 
`join`. Of course, then you read what's next, and you discover that: (a) 
shutdown might be called by a custom scope subclass and that (b) shutdown is 
confined to the threads contained in this task scope - but this definition is 
only given much later.

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

> 351:  *
> 352:  *  The {@code handleComplete} method should be thread safe. It 
> may be
> 353:  * invoked by several threads at around the same.

Something is missing? E.g. "at around the same TIME" ? (I'd suggest just using 
"concurrently")

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

> 374:  *
> 375:  *  If this task scope is {@linkplain #shutdown() shutdown} (or 
> in the process
> 376:  * of shutting down) then {@code fork} returns a Future representing 
> a {@link

Future in plaintext?

-

PR: https://git.openjdk.java.net/jdk/pull/8787


Re: RFR: 8284199: Implementation of Structured Concurrency (Incubator)

2022-05-23 Thread Paul Sandoz
On Sat, 21 May 2022 16:25:57 GMT, Alan Bateman  wrote:

>> src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/StructuredTaskScope.java
>>  line 1172:
>> 
>>> 1170: }
>>> 1171: };
>>> 1172: return AccessController.doPrivileged(pa);
>> 
>> It might be better to use `MethodHandle`s obtained using 
>> [jdk.internal.access.SharedSecrets].getJavaLangInvokeAccess()
>>  and [JavaLangInvokeAccess].findStatic(…) and 
>> [JavaLangInvokeAccess].findVirtual(…) for this, which 
>> would avoid going through `AccessController.doPrivilaged(…)`.
>> 
>> [jdk.internal.access.SharedSecrets]: 
>> https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/jdk/internal/access/SharedSecrets.java
>> [JavaLangInvokeAccess]: 
>> https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/jdk/internal/access/JavaLangInvokeAccess.java
>
> I'd prefer not export jdk.internal.access to this module, if possible.  In 
> general, it's a lot easier to reason about the security and integrity of the 
> core platform when java.base doesn't export any of its internal packages.
> 
> In any case, I expect this issue will resolve itself once there is a way for 
> code in "core" modules to use preview APIs without needing to be compiled 
> with --enable-preview. The java.management and jdk.incubator.vector modules 
> have the same issue.

Yes, we will add a more general internal for source that participate in preview 
APIs, and then the use of reflection can be removed. (The Vector API PR has a 
focused fix to the compiler, since it cannot use the reflection trick).

-

PR: https://git.openjdk.java.net/jdk/pull/8787


Re: RFR: 8284199: Implementation of Structured Concurrency (Incubator)

2022-05-23 Thread Paul Sandoz
On Thu, 19 May 2022 13:05:54 GMT, Alan Bateman  wrote:

> This is the implementation of JEP 428: Structured Concurrency (Incubator).
> 
> This is a non-final API that provides a gentle on-ramp to structure a task as 
> a family of concurrent subtasks, and to coordinate the subtasks as a unit.

Previously reviewed while in the loom repo.

src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/StructureViolationException.java
 line 40:

> 38: 
> 39: /**
> 40:  * Constructs an {@code StructureViolationException} with no detail 
> message.

Suggestion:

 * Constructs a {@code StructureViolationException} with no detail message.

src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/StructureViolationException.java
 line 47:

> 45: 
> 46: /**
> 47:  * Constructs an {@code StructureViolationException} with the 
> specified

Suggestion:

 * Constructs a {@code StructureViolationException} with the specified

-

Marked as reviewed by psandoz (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/8787


Re: RFR: 8284199: Implementation of Structured Concurrency (Incubator)

2022-05-23 Thread Daniel Fuchs
On Thu, 19 May 2022 13:05:54 GMT, Alan Bateman  wrote:

> This is the implementation of JEP 428: Structured Concurrency (Incubator).
> 
> This is a non-final API that provides a gentle on-ramp to structure a task as 
> a family of concurrent subtasks, and to coordinate the subtasks as a unit.

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

> 52: 
> 53: /**
> 54:  * A basic API for structured concurrency. StructuredTaskScope 
> supports cases

Should StructuredTaskScope in this class-level API doc comment be surrounded by 
`{@code }` to appear in code font?

-

PR: https://git.openjdk.java.net/jdk/pull/8787


Re: RFR: 8284199: Implementation of Structured Concurrency (Incubator)

2022-05-23 Thread ExE Boss
On Thu, 19 May 2022 13:05:54 GMT, Alan Bateman  wrote:

> This is the implementation of JEP 428: Structured Concurrency (Incubator).
> 
> This is a non-final API that provides a gentle on-ramp to structure a task as 
> a family of concurrent subtasks, and to coordinate the subtasks as a unit.

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

> 1170: }
> 1171: };
> 1172: return AccessController.doPrivileged(pa);

It might be better to use `MethodHandle`s obtained using 
[jdk.internal.access.SharedSecrets].getJavaLangInvokeAccess()
 and [JavaLangInvokeAccess].findStatic(…) and 
[JavaLangInvokeAccess].findVirtual(…) for this, which 
would avoid going through `AccessController.doPrivilaged(…)`.

[jdk.internal.access.SharedSecrets]: 
https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/jdk/internal/access/SharedSecrets.java
[JavaLangInvokeAccess]: 
https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/jdk/internal/access/JavaLangInvokeAccess.java

-

PR: https://git.openjdk.java.net/jdk/pull/8787


Re: RFR: 8284199: Implementation of Structured Concurrency (Incubator)

2022-05-23 Thread Alan Bateman
On Sat, 21 May 2022 14:09:59 GMT, ExE Boss  wrote:

>> This is the implementation of JEP 428: Structured Concurrency (Incubator).
>> 
>> This is a non-final API that provides a gentle on-ramp to structure a task 
>> as a family of concurrent subtasks, and to coordinate the subtasks as a unit.
>
> src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/StructuredTaskScope.java
>  line 1172:
> 
>> 1170: }
>> 1171: };
>> 1172: return AccessController.doPrivileged(pa);
> 
> It might be better to use `MethodHandle`s obtained using 
> [jdk.internal.access.SharedSecrets].getJavaLangInvokeAccess()
>  and [JavaLangInvokeAccess].findStatic(…) and 
> [JavaLangInvokeAccess].findVirtual(…) for this, which 
> would avoid going through `AccessController.doPrivilaged(…)`.
> 
> [jdk.internal.access.SharedSecrets]: 
> https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/jdk/internal/access/SharedSecrets.java
> [JavaLangInvokeAccess]: 
> https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/jdk/internal/access/JavaLangInvokeAccess.java

I'd prefer not export jdk.internal.access to this module, if possible.  In 
general, it's a lot easier to reason about the security and integrity of the 
core platform when java.base doesn't export any of its internal packages.

In any case, I expect this issue will resolve itself once there is a way for 
code in "core" modules to use preview APIs without needing to be compiled with 
--enable-preview. The java.management and jdk.incubator.vector modules have the 
same issue.

-

PR: https://git.openjdk.java.net/jdk/pull/8787