Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v3]
On Sun, 17 Apr 2022 04:57:34 GMT, Jaikiran Pai 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 five additional
>> commits since the last revision:
>>
>> - Refresh
>> - Refresh
>> - Merge with jdk-19+18
>> - Refresh
>> - Initial push
>
> src/java.base/share/classes/java/lang/StackTraceElement.java line 556:
>
>> 554: }
>> 555:
>> 556: static StackTraceElement[] of(StackTraceElement[] stackTrace) {
>
> Is it intentional that this method is returning a `StackTraceElement[]`
> instead of `void`. The current implementation just returns back the passed
> array after operating on each element. Are callers mandated to use the
> returned array for subsequent operations instead of the passed array?
It's just for convenience of use.
-
PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v3]
On Sun, 17 Apr 2022 12:39:24 GMT, Jaikiran Pai wrote:
> I had a look at the referenced
> [JDK-4006245](https://bugs.openjdk.java.net/browse/JDK-4006245). The
> previous/existing reference cleaning logic seems to have been added mainly to
> `null` the reference to the user/application class instance represented by
> the `Runnable` task. Of course, other references too were being cleared. The
> description in the linked JBS seems to suggest that the `Thread` instance
> wasn't being GCed sooner. That was more than 2 decades back. I don't have
> enough knowledge of this area to know if this is still an issue in recent GC
> implementations.
The comment on that method dates from early JDK releases with the Classic VM
and its conservative GC. It also predates the java.lang.ref API. The question
that I think you are asking is if you create a Thread with a runnable task, let
it terminate but keep the reference to the Thread forever then what does it
keep alive. We can think about more but so far we haven't seen any issues or
tests fail with the reference to the runnable task.
> src/java.base/share/classes/java/lang/Thread.java line 2414:
>
>> 2412: sm.checkPermission(new
>> RuntimePermission("setContextClassLoader"));
>> 2413: }
>> 2414: if (!isSupportedClassLoader(contextClassLoader)) {
>
> For a few moments, I thought this was a typo and should instead have been:
>
> if (!isSupportedClassLoader(cl)) {
>
> but then thinking a bit more, I realized what's being done here. Essentially,
> this checks whether the _current_ context classloader is set to "not allowed
> to change/set context classloader" (represented by
> `Constants.NOT_SUPPORTED_CLASSLOADER`).
>
> Do you think this line deserves some short code comment that it intentionally
> passes the current contextClassLoader (and not the one passed to the method)
> to `isSupportedClassLoader`?
I think the comment on isSupportedClassLoader is readable so I think it's okay.
-
PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v3]
On Fri, 15 Apr 2022 11:45:01 GMT, Alan Bateman wrote:
>> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which
>> JDK version to target.
>>
>> We will refresh this PR periodically to pick up changes and fixes from the
>> loom repo.
>>
>> Most of the new mechanisms in the HotSpot VM are disabled by default and
>> require running with `--enable-preview` to enable.
>>
>> The patch has support for x64 and aarch64 on the usual operating systems
>> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for
>> zero and some of the other ports. Additional ports can be contributed via
>> PRs against the fibers branch in the loom repo.
>>
>> There are changes in many areas. To reduce notifications/mails, the labels
>> have been trimmed down for now to hotspot, serviceability and core-libs.
>> We'll add the complete set of labels when the PR is further along.
>>
>> The changes include a refresh of java.util.concurrent and ForkJoinPool from
>> Doug Lea's CVS. These changes will probably be proposed and integrated in
>> advance of this PR.
>>
>> The changes include some non-exposed and low-level infrastructure to support
>> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to
>> make life a bit easier and avoid having to separate VM changes and juggle
>> branches at this time.
>
> 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 five additional
> commits since the last revision:
>
> - Refresh
> - Refresh
> - Merge with jdk-19+18
> - Refresh
> - Initial push
src/java.base/share/classes/java/lang/Thread.java line 2414:
> 2412: sm.checkPermission(new
> RuntimePermission("setContextClassLoader"));
> 2413: }
> 2414: if (!isSupportedClassLoader(contextClassLoader)) {
For a few moments, I thought this was a typo and should instead have been:
if (!isSupportedClassLoader(cl)) {
but then thinking a bit more, I realized what's being done here. Essentially,
this checks whether the _current_ context classloader is set to "not allowed
to change/set context classloader" (represented by
`Constants.NOT_SUPPORTED_CLASSLOADER`).
Do you think this line deserves some short code comment that it intentionally
passes the current contextClassLoader (and not the one passed to the method) to
`isSupportedClassLoader`?
-
PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v3]
On Fri, 15 Apr 2022 11:45:01 GMT, Alan Bateman wrote:
>> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which
>> JDK version to target.
>>
>> We will refresh this PR periodically to pick up changes and fixes from the
>> loom repo.
>>
>> Most of the new mechanisms in the HotSpot VM are disabled by default and
>> require running with `--enable-preview` to enable.
>>
>> The patch has support for x64 and aarch64 on the usual operating systems
>> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for
>> zero and some of the other ports. Additional ports can be contributed via
>> PRs against the fibers branch in the loom repo.
>>
>> There are changes in many areas. To reduce notifications/mails, the labels
>> have been trimmed down for now to hotspot, serviceability and core-libs.
>> We'll add the complete set of labels when the PR is further along.
>>
>> The changes include a refresh of java.util.concurrent and ForkJoinPool from
>> Doug Lea's CVS. These changes will probably be proposed and integrated in
>> advance of this PR.
>>
>> The changes include some non-exposed and low-level infrastructure to support
>> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to
>> make life a bit easier and avoid having to separate VM changes and juggle
>> branches at this time.
>
> 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 five additional
> commits since the last revision:
>
> - Refresh
> - Refresh
> - Merge with jdk-19+18
> - Refresh
> - Initial push
src/java.base/share/classes/java/lang/Thread.java line 1554:
> 1552: * Null out reference after Thread termination (JDK-4006245)
> 1553: */
> 1554: void clearReferences() {
I had a look at the referenced
[JDK-4006245](https://bugs.openjdk.java.net/browse/JDK-4006245). The
previous/existing reference cleaning logic seems to have been added mainly to
`null` the reference to the user/application class instance represented by the
`Runnable` task. Of course, other references too were being cleared. The
description in the linked JBS seems to suggest that the `Thread` instance
wasn't being GCed sooner. That was more than 2 decades back. I don't have
enough knowledge of this area to know if this is still an issue in recent GC
implementations.
With the new implementation in this PR, this `clearReferences()` will no longer
set the user/application `task` instance to `null`. I understand why we aren't
doing that - the `task` field in the `FieldHolder` is `final` (IMO, rightly
so). This, in theory, undoes what JDK-4006245 had implemented. I don't know if
we still need such a change here and I'm just bringing this up to see if anyone
else has any opinions or if any additional experiments need to be run in
context of this change.
-
PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v3]
On Sun, 17 Apr 2022 03:49:19 GMT, Jaikiran Pai 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 five additional
>> commits since the last revision:
>>
>> - Refresh
>> - Refresh
>> - Merge with jdk-19+18
>> - Refresh
>> - Initial push
>
> src/java.base/share/classes/java/io/PrintStream.java line 1420:
>
>> 1418: } else {
>> 1419: synchronized (this) {
>> 1420: implFormat(format, args);
>
> I think there's a typo here. I think it should have been:
>
>
> implFormat(l, format, args);
Well spotted, the locale should be provided.
> src/java.base/share/classes/java/lang/Thread.java line 602:
>
>> 600: } else {
>> 601: // getContextClassLoader not trusted
>> 602: ClassLoader cl = parent.contextClassLoader;
>
> I might be misreading the comment on that line, but reading this if/else
> block a few times, I'm unsure what the expectations here are.
>
> It's my understanding that a call to `getContextClassLoader()` can't be
> trusted if that method has been overridden by the passed `Thread` type. In
> such cases, we don't call that method and instead just use the field value of
> `contextClassLoader` (which is a private field on `Thread`). Is that
> understanding correct? If yes, then the condition in the `if` block a few
> lines above, looks odd. It seems to be calling the `getContextClassLoader()`
> if that method is overridden by the passed `Thread` type, i.e. the untrusted
> case. Should it instead be:
>
> if (sm == null || !isCCLOverridden(parent.getClass())) {
> return parent.getContextClassLoader();
> }
>
> (notice the negation)
This code hasn't changed, it's just moved to a helper method. When running with
a security manager and the CCL methods aren't overridden, then it avoids the
permission check. However, I can see how the comment is mis-reading so we can
improve that.
-
PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v3]
On Fri, 15 Apr 2022 11:45:01 GMT, Alan Bateman wrote:
>> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which
>> JDK version to target.
>>
>> We will refresh this PR periodically to pick up changes and fixes from the
>> loom repo.
>>
>> Most of the new mechanisms in the HotSpot VM are disabled by default and
>> require running with `--enable-preview` to enable.
>>
>> The patch has support for x64 and aarch64 on the usual operating systems
>> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for
>> zero and some of the other ports. Additional ports can be contributed via
>> PRs against the fibers branch in the loom repo.
>>
>> There are changes in many areas. To reduce notifications/mails, the labels
>> have been trimmed down for now to hotspot, serviceability and core-libs.
>> We'll add the complete set of labels when the PR is further along.
>>
>> The changes include a refresh of java.util.concurrent and ForkJoinPool from
>> Doug Lea's CVS. These changes will probably be proposed and integrated in
>> advance of this PR.
>>
>> The changes include some non-exposed and low-level infrastructure to support
>> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to
>> make life a bit easier and avoid having to separate VM changes and juggle
>> branches at this time.
>
> 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 five additional
> commits since the last revision:
>
> - Refresh
> - Refresh
> - Merge with jdk-19+18
> - Refresh
> - Initial push
src/java.base/share/classes/java/lang/Thread.java line 602:
> 600: } else {
> 601: // getContextClassLoader not trusted
> 602: ClassLoader cl = parent.contextClassLoader;
I might be misreading the comment on that line, but reading this if/else block
a few times, I'm unsure what the expectations here are.
It's my understanding that a call to `getContextClassLoader()` can't be trusted
if that method has been overridden by the passed `Thread` type. In such cases,
we don't call that method and instead just use the field value of
`contextClassLoader` (which is a private field on `Thread`). Is that
understanding correct? If yes, then the condition in the `if` block a few lines
above, looks odd. It seems to be calling the `getContextClassLoader()` if that
method is overridden by the passed `Thread` type, i.e. the untrusted case.
Should it instead be:
if (sm == null || !isCCLOverridden(parent.getClass())) {
return parent.getContextClassLoader();
}
(notice the negation)
-
PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v3]
On Fri, 15 Apr 2022 11:45:01 GMT, Alan Bateman wrote:
>> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which
>> JDK version to target.
>>
>> We will refresh this PR periodically to pick up changes and fixes from the
>> loom repo.
>>
>> Most of the new mechanisms in the HotSpot VM are disabled by default and
>> require running with `--enable-preview` to enable.
>>
>> The patch has support for x64 and aarch64 on the usual operating systems
>> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for
>> zero and some of the other ports. Additional ports can be contributed via
>> PRs against the fibers branch in the loom repo.
>>
>> There are changes in many areas. To reduce notifications/mails, the labels
>> have been trimmed down for now to hotspot, serviceability and core-libs.
>> We'll add the complete set of labels when the PR is further along.
>>
>> The changes include a refresh of java.util.concurrent and ForkJoinPool from
>> Doug Lea's CVS. These changes will probably be proposed and integrated in
>> advance of this PR.
>>
>> The changes include some non-exposed and low-level infrastructure to support
>> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to
>> make life a bit easier and avoid having to separate VM changes and juggle
>> branches at this time.
>
> 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 five additional
> commits since the last revision:
>
> - Refresh
> - Refresh
> - Merge with jdk-19+18
> - Refresh
> - Initial push
src/java.base/share/classes/java/lang/StackTraceElement.java line 556:
> 554: }
> 555:
> 556: static StackTraceElement[] of(StackTraceElement[] stackTrace) {
Is it intentional that this method is returning a `StackTraceElement[]` instead
of `void`. The current implementation just returns back the passed array after
operating on each element. Are callers mandated to use the returned array for
subsequent operations instead of the passed array?
-
PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v3]
On Fri, 15 Apr 2022 11:45:01 GMT, Alan Bateman wrote:
>> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which
>> JDK version to target.
>>
>> We will refresh this PR periodically to pick up changes and fixes from the
>> loom repo.
>>
>> Most of the new mechanisms in the HotSpot VM are disabled by default and
>> require running with `--enable-preview` to enable.
>>
>> The patch has support for x64 and aarch64 on the usual operating systems
>> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for
>> zero and some of the other ports. Additional ports can be contributed via
>> PRs against the fibers branch in the loom repo.
>>
>> There are changes in many areas. To reduce notifications/mails, the labels
>> have been trimmed down for now to hotspot, serviceability and core-libs.
>> We'll add the complete set of labels when the PR is further along.
>>
>> The changes include a refresh of java.util.concurrent and ForkJoinPool from
>> Doug Lea's CVS. These changes will probably be proposed and integrated in
>> advance of this PR.
>>
>> The changes include some non-exposed and low-level infrastructure to support
>> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to
>> make life a bit easier and avoid having to separate VM changes and juggle
>> branches at this time.
>
> 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 five additional
> commits since the last revision:
>
> - Refresh
> - Refresh
> - Merge with jdk-19+18
> - Refresh
> - Initial push
src/java.base/share/classes/java/io/PrintStream.java line 1420:
> 1418: } else {
> 1419: synchronized (this) {
> 1420: implFormat(format, args);
I think there's a typo here. I think it should have been:
implFormat(l, format, args);
-
PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v3]
On Sat, 16 Apr 2022 14:34:01 GMT, Alan Bateman wrote:
>> src/java.base/share/classes/jdk/internal/misc/Blocker.java line 76:
>>
>>> 74: && currentCarrierThread() instanceof CarrierThread ct
>>> && !ct.inBlocking()) {
>>> 75: ct.beginBlocking();
>>> 76: long comp =
>>> ForkJoinPools.beginCompensatedBlock(ct.getPool());
>>
>> It appears that `ForkJoinPools.beginCompensatedBlock(...)` can throw an
>> exception. Would such an exception require the CarrierThread's blocking
>> state (which we set on the previous line) to be reset by a call to
>> `ct.endBlocking()`? Or is it futile to reset that state if any exception
>> gets thrown from the call to `ForkJoinPools.beginCompensatedBlock(...)`?
>
> REE isn't possible here but maybe you mean a resource issues such as stack
> overflow or OOME. If that happens then the flag wouldn't be reset. It
> wouldn't a correctness issue but we may be able to make this a bit more
> robust for these cases.
Hello Alan,
My concern wasn't really about OOME or resource issue. I had noticed the
`ForkJoinPools.beginCompensatedBlock` has a try/catch which then propagates
back the `Throwable`. So I thought there could be some genuine exceptions that
could be thrown in this flow which ultimately calls
`ForkJoinPool.beginCompensatedBlock()`. I haven't yet reached the changes in
`ForkJoinPool.java` class itself, so it's likely that this catch block is only
there for theoretical reasons when dealing with `MethodHandle`.
-
PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v3]
On Sat, 16 Apr 2022 09:25:20 GMT, Jaikiran Pai 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 five additional
>> commits since the last revision:
>>
>> - Refresh
>> - Refresh
>> - Merge with jdk-19+18
>> - Refresh
>> - Initial push
>
> src/java.base/share/classes/java/io/BufferedOutputStream.java line 136:
>
>> 134: * are added. A no-op if the buffer is not resizable.
>> 135: */
>> 136: private void growIfNeeded(int len) {
>
> Hello Alan, like some methods in other classes (BufferedInputStream#fill()
> for example), should we mention here that this method assumes that it's being
> called while a lock is held?
Okay, we can add a comment to these methods (asserts are possible but tend not
to be used in this area).
-
PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v3]
On Sat, 16 Apr 2022 11:05:47 GMT, Jaikiran Pai 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 five additional
>> commits since the last revision:
>>
>> - Refresh
>> - Refresh
>> - Merge with jdk-19+18
>> - Refresh
>> - Initial push
>
> src/java.base/share/classes/java/io/FilterInputStream.java line 233:
>
>> 231: */
>> 232: @Override
>> 233: public void reset() throws IOException {
>
> I suspect this change to remove synchronization from `mark()` and `reset()`
> is intentional. The `FilterInputStream` class doesn't explain the thread
> safety semantics, nor can I find commit history which would hint on why these
> (and only these) methods were syncrhonized. But it looks like this
> synchronization was there to serialize calls to `mark()` and `reset()` on the
> same `FilterInputStream` from multiple threads. With this synchronization now
> removed, subclasses or even direct usages of `FilterInputStream` would now be
> impacted. Potentially outside of the JDK. Would that be a concern?
Yes, this is intentional but it would be great to split out this change and get
them into the main line in advance. There is a JEP size task required to
re-visit all of the locking in java.io. In the case of the input/output
streams, most of it is not documented and there are several inconsistencies. In
the case of FilterInputStream, it doesn't use synchronization for the other ops.
-
PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v3]
On Sat, 16 Apr 2022 10:25:56 GMT, Jaikiran Pai 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 five additional
>> commits since the last revision:
>>
>> - Refresh
>> - Refresh
>> - Merge with jdk-19+18
>> - Refresh
>> - Initial push
>
> src/java.base/share/classes/jdk/internal/misc/Blocker.java line 76:
>
>> 74: && currentCarrierThread() instanceof CarrierThread ct &&
>> !ct.inBlocking()) {
>> 75: ct.beginBlocking();
>> 76: long comp =
>> ForkJoinPools.beginCompensatedBlock(ct.getPool());
>
> It appears that `ForkJoinPools.beginCompensatedBlock(...)` can throw an
> exception. Would such an exception require the CarrierThread's blocking state
> (which we set on the previous line) to be reset by a call to
> `ct.endBlocking()`? Or is it futile to reset that state if any exception gets
> thrown from the call to `ForkJoinPools.beginCompensatedBlock(...)`?
REE isn't possible here but maybe you mean a resource issues such as stack
overflow or OOME. If that happens then the flag wouldn't be reset. It wouldn't
a corrects issue but we may be able to make this a bit more robust for these
cases.
-
PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v3]
On Sat, 16 Apr 2022 10:29:36 GMT, Jaikiran Pai 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 five additional >> commits since the last revision: >> >> - Refresh >> - Refresh >> - Merge with jdk-19+18 >> - Refresh >> - Initial push > > src/java.base/share/classes/jdk/internal/misc/Blocker.java line 94: > >> 92: >> 93: /** >> 94: * Marks the end an operation that may have blocked. > > I think this should have been "Marks the end of an ..." Yes, there's a typo there - thanks. - PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v3]
On Fri, 15 Apr 2022 11:45:01 GMT, Alan Bateman wrote:
>> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which
>> JDK version to target.
>>
>> We will refresh this PR periodically to pick up changes and fixes from the
>> loom repo.
>>
>> Most of the new mechanisms in the HotSpot VM are disabled by default and
>> require running with `--enable-preview` to enable.
>>
>> The patch has support for x64 and aarch64 on the usual operating systems
>> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for
>> zero and some of the other ports. Additional ports can be contributed via
>> PRs against the fibers branch in the loom repo.
>>
>> There are changes in many areas. To reduce notifications/mails, the labels
>> have been trimmed down for now to hotspot, serviceability and core-libs.
>> We'll add the complete set of labels when the PR is further along.
>>
>> The changes include a refresh of java.util.concurrent and ForkJoinPool from
>> Doug Lea's CVS. These changes will probably be proposed and integrated in
>> advance of this PR.
>>
>> The changes include some non-exposed and low-level infrastructure to support
>> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to
>> make life a bit easier and avoid having to separate VM changes and juggle
>> branches at this time.
>
> 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 five additional
> commits since the last revision:
>
> - Refresh
> - Refresh
> - Merge with jdk-19+18
> - Refresh
> - Initial push
src/java.base/share/classes/java/io/FilterInputStream.java line 233:
> 231: */
> 232: @Override
> 233: public void reset() throws IOException {
I suspect this change to remove synchronization from `mark()` and `reset()` is
intentional. The `FilterInputStream` class doesn't explain the thread safety
semantics, nor can I find commit history which would hint on why these (and
only these) methods were syncrhonized. But it looks like this synchronization
was there to serialize calls to `mark()` and `reset()` on the same
`FilterInputStream` from multiple threads. With this synchronization now
removed, subclasses or even direct usages of `FilterInputStream` would now be
impacted. Potentially outside of the JDK. Would that be a concern?
-
PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v3]
On Fri, 15 Apr 2022 11:45:01 GMT, Alan Bateman wrote:
>> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which
>> JDK version to target.
>>
>> We will refresh this PR periodically to pick up changes and fixes from the
>> loom repo.
>>
>> Most of the new mechanisms in the HotSpot VM are disabled by default and
>> require running with `--enable-preview` to enable.
>>
>> The patch has support for x64 and aarch64 on the usual operating systems
>> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for
>> zero and some of the other ports. Additional ports can be contributed via
>> PRs against the fibers branch in the loom repo.
>>
>> There are changes in many areas. To reduce notifications/mails, the labels
>> have been trimmed down for now to hotspot, serviceability and core-libs.
>> We'll add the complete set of labels when the PR is further along.
>>
>> The changes include a refresh of java.util.concurrent and ForkJoinPool from
>> Doug Lea's CVS. These changes will probably be proposed and integrated in
>> advance of this PR.
>>
>> The changes include some non-exposed and low-level infrastructure to support
>> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to
>> make life a bit easier and avoid having to separate VM changes and juggle
>> branches at this time.
>
> 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 five additional
> commits since the last revision:
>
> - Refresh
> - Refresh
> - Merge with jdk-19+18
> - Refresh
> - Initial push
src/java.base/share/classes/jdk/internal/misc/Blocker.java line 76:
> 74: && currentCarrierThread() instanceof CarrierThread ct &&
> !ct.inBlocking()) {
> 75: ct.beginBlocking();
> 76: long comp = ForkJoinPools.beginCompensatedBlock(ct.getPool());
It appears that `ForkJoinPools.beginCompensatedBlock(...)` can throw an
exception. Would such an exception require the CarrierThread's blocking state
(which we set on the previous line) to be reset by a call to
`ct.endBlocking()`? Or is it futile to reset that state if any exception gets
thrown from the call to `ForkJoinPools.beginCompensatedBlock(...)`?
src/java.base/share/classes/jdk/internal/misc/Blocker.java line 94:
> 92:
> 93: /**
> 94: * Marks the end an operation that may have blocked.
I think this should have been "Marks the end of an ..."
-
PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v3]
On Fri, 15 Apr 2022 11:45:01 GMT, Alan Bateman wrote:
>> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which
>> JDK version to target.
>>
>> We will refresh this PR periodically to pick up changes and fixes from the
>> loom repo.
>>
>> Most of the new mechanisms in the HotSpot VM are disabled by default and
>> require running with `--enable-preview` to enable.
>>
>> The patch has support for x64 and aarch64 on the usual operating systems
>> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for
>> zero and some of the other ports. Additional ports can be contributed via
>> PRs against the fibers branch in the loom repo.
>>
>> There are changes in many areas. To reduce notifications/mails, the labels
>> have been trimmed down for now to hotspot, serviceability and core-libs.
>> We'll add the complete set of labels when the PR is further along.
>>
>> The changes include a refresh of java.util.concurrent and ForkJoinPool from
>> Doug Lea's CVS. These changes will probably be proposed and integrated in
>> advance of this PR.
>>
>> The changes include some non-exposed and low-level infrastructure to support
>> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to
>> make life a bit easier and avoid having to separate VM changes and juggle
>> branches at this time.
>
> 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 five additional
> commits since the last revision:
>
> - Refresh
> - Refresh
> - Merge with jdk-19+18
> - Refresh
> - Initial push
src/java.base/share/classes/java/io/BufferedOutputStream.java line 136:
> 134: * are added. A no-op if the buffer is not resizable.
> 135: */
> 136: private void growIfNeeded(int len) {
Hello Alan, like some methods in other classes (BufferedInputStream#fill() for
example), should we mention here that this method assumes that it's being
called while a lock is held?
-
PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v3]
On Fri, 15 Apr 2022 12:31:24 GMT, Andrey Turbanov wrote: > method seems unused now It's used by the SL API, which will follow later. I realize this may sound complicated but it avoids more complicated splits. - PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v3]
On Fri, 15 Apr 2022 11:45:01 GMT, Alan Bateman wrote:
>> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which
>> JDK version to target.
>>
>> We will refresh this PR periodically to pick up changes and fixes from the
>> loom repo.
>>
>> Most of the new mechanisms in the HotSpot VM are disabled by default and
>> require running with `--enable-preview` to enable.
>>
>> The patch has support for x64 and aarch64 on the usual operating systems
>> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for
>> zero and some of the other ports. Additional ports can be contributed via
>> PRs against the fibers branch in the loom repo.
>>
>> There are changes in many areas. To reduce notifications/mails, the labels
>> have been trimmed down for now to hotspot, serviceability and core-libs.
>> We'll add the complete set of labels when the PR is further along.
>>
>> The changes include a refresh of java.util.concurrent and ForkJoinPool from
>> Doug Lea's CVS. These changes will probably be proposed and integrated in
>> advance of this PR.
>>
>> The changes include some non-exposed and low-level infrastructure to support
>> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to
>> make life a bit easier and avoid having to separate VM changes and juggle
>> branches at this time.
>
> 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 five additional
> commits since the last revision:
>
> - Refresh
> - Refresh
> - Merge with jdk-19+18
> - Refresh
> - Initial push
src/java.base/share/classes/jdk/internal/access/JavaUtilConcurrentTLRAccess.java
line 29:
> 27:
> 28: public interface JavaUtilConcurrentTLRAccess {
> 29: int nextSecondaryThreadLocalRandomSeed();
method seems unused now
-
PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v3]
> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which > JDK version to target. > > We will refresh this PR periodically to pick up changes and fixes from the > loom repo. > > Most of the new mechanisms in the HotSpot VM are disabled by default and > require running with `--enable-preview` to enable. > > The patch has support for x64 and aarch64 on the usual operating systems > (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for zero > and some of the other ports. Additional ports can be contributed via PRs > against the fibers branch in the loom repo. > > There are changes in many areas. To reduce notifications/mails, the labels > have been trimmed down for now to hotspot, serviceability and core-libs. > We'll add the complete set of labels when the PR is further along. > > The changes include a refresh of java.util.concurrent and ForkJoinPool from > Doug Lea's CVS. These changes will probably be proposed and integrated in > advance of this PR. > > The changes include some non-exposed and low-level infrastructure to support > the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to > make life a bit easier and avoid having to separate VM changes and juggle > branches at this time. 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 five additional commits since the last revision: - Refresh - Refresh - Merge with jdk-19+18 - Refresh - Initial push - Changes: - all: https://git.openjdk.java.net/jdk/pull/8166/files - new: https://git.openjdk.java.net/jdk/pull/8166/files/0e12c206..f53f0d4e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8166&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8166&range=01-02 Stats: 148912 lines in 1502 files changed: 104901 ins; 9917 del; 34094 mod Patch: https://git.openjdk.java.net/jdk/pull/8166.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8166/head:pull/8166 PR: https://git.openjdk.java.net/jdk/pull/8166
