Re: RFR: 8284199: Implementation of Structured Concurrency (Incubator) [v2]
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]
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]
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]
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]
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]
> 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
