Re: RFR: 8329593: Drop adjustments to target parallelism when virtual threads do I/O on files opened for buffered I/O [v2]
On Tue, 23 Apr 2024 08:09:20 GMT, Alan Bateman wrote:
>> src/java.base/share/classes/java/io/FileInputStream.java line 211:
>>
>>> 209: * @param name the name of the file
>>> 210: */
>>> 211: private void open(String name) throws FileNotFoundException {
>>
>> If method such as this is private, and only delegates to the 0-suffixed
>> native method, would't it be better to just call the 0-suffixed method
>> directly?
>
> Historically these native methods were wrapped in order to support
> instrumentation, I didn't want to touch in this PR.
And presumably all these 0-suffixed methods will eventually be replaced with
FFM calls anyway.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18598#discussion_r1576561165
Re: RFR: 8329593: Drop adjustments to target parallelism when virtual threads do I/O on files opened for buffered I/O [v2]
On Tue, 23 Apr 2024 08:04:54 GMT, Viktor Klang 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:
>>
>> - Merge
>> - Sync up from loom repo, update copyright headers
>> - Merge
>> - Merge
>> - Initial commit
>
> src/java.base/share/classes/java/io/FileInputStream.java line 211:
>
>> 209: * @param name the name of the file
>> 210: */
>> 211: private void open(String name) throws FileNotFoundException {
>
> If method such as this is private, and only delegates to the 0-suffixed
> native method, would't it be better to just call the 0-suffixed method
> directly?
Historically these native methods were wrapped in order to support
instrumentation, I didn't want to touch in this PR.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18598#discussion_r1575831563
Re: RFR: 8329593: Drop adjustments to target parallelism when virtual threads do I/O on files opened for buffered I/O [v2]
On Mon, 22 Apr 2024 12:45:53 GMT, Alan Bateman wrote:
>> This change drops the adjustments to the virtual thread scheduler's target
>> parallelism when virtual threads do file operations on files that are opened
>> for buffered I/O. These operations are usually just too short to have any
>> benefit and may have a negative benefit when reading/writing a small number
>> of bytes. There is no change for read/write operations on files opened for
>> direct I/O or when writing to files that are opened with options for
>> synchronized I/O file integrity (O_SYNC/O_DSYNC and equivalents). Sergery
>> Kuksenko is polishing benchmarks that includes this area, this is for a
>> future PR.
>>
>> In addition, the blocker mechanism is updated to handle reentrancy (as can
>> happen if debugging code is added to ForkJoinPool) and preemption when
>> compensating (as can happen when substituting a heap buffer with a direct
>> buffer in some I/O operations). This part is a pre-requisite to the changes
>> to better support object monitor there are more places where preemption is
>> possible and this quickly leads to unbalanced begin/end.
>>
>> The changes have been baking in loom repo for some 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:
>
> - Merge
> - Sync up from loom repo, update copyright headers
> - Merge
> - Merge
> - Initial commit
src/java.base/share/classes/java/io/FileInputStream.java line 211:
> 209: * @param name the name of the file
> 210: */
> 211: private void open(String name) throws FileNotFoundException {
If method such as this is private, and only delegates to the 0-suffixed native
method, would't it be better to just call the 0-suffixed method directly?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18598#discussion_r1575825688
Re: RFR: 8329593: Drop adjustments to target parallelism when virtual threads do I/O on files opened for buffered I/O [v2]
On Mon, 22 Apr 2024 12:45:53 GMT, Alan Bateman wrote: >> This change drops the adjustments to the virtual thread scheduler's target >> parallelism when virtual threads do file operations on files that are opened >> for buffered I/O. These operations are usually just too short to have any >> benefit and may have a negative benefit when reading/writing a small number >> of bytes. There is no change for read/write operations on files opened for >> direct I/O or when writing to files that are opened with options for >> synchronized I/O file integrity (O_SYNC/O_DSYNC and equivalents). Sergery >> Kuksenko is polishing benchmarks that includes this area, this is for a >> future PR. >> >> In addition, the blocker mechanism is updated to handle reentrancy (as can >> happen if debugging code is added to ForkJoinPool) and preemption when >> compensating (as can happen when substituting a heap buffer with a direct >> buffer in some I/O operations). This part is a pre-requisite to the changes >> to better support object monitor there are more places where preemption is >> possible and this quickly leads to unbalanced begin/end. >> >> The changes have been baking in loom repo for some 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: > > - Merge > - Sync up from loom repo, update copyright headers > - Merge > - Merge > - Initial commit Hello Alan, the latest changes look fine to me. They also address the review comments from the previous iteration. - Marked as reviewed by jpai (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18598#pullrequestreview-2016427528
Re: RFR: 8329593: Drop adjustments to target parallelism when virtual threads do I/O on files opened for buffered I/O [v2]
On Mon, 22 Apr 2024 12:45:53 GMT, Alan Bateman wrote: >> This change drops the adjustments to the virtual thread scheduler's target >> parallelism when virtual threads do file operations on files that are opened >> for buffered I/O. These operations are usually just too short to have any >> benefit and may have a negative benefit when reading/writing a small number >> of bytes. There is no change for read/write operations on files opened for >> direct I/O or when writing to files that are opened with options for >> synchronized I/O file integrity (O_SYNC/O_DSYNC and equivalents). Sergery >> Kuksenko is polishing benchmarks that includes this area, this is for a >> future PR. >> >> In addition, the blocker mechanism is updated to handle reentrancy (as can >> happen if debugging code is added to ForkJoinPool) and preemption when >> compensating (as can happen when substituting a heap buffer with a direct >> buffer in some I/O operations). This part is a pre-requisite to the changes >> to better support object monitor there are more places where preemption is >> possible and this quickly leads to unbalanced begin/end. >> >> The changes have been baking in loom repo for some 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: > > - Merge > - Sync up from loom repo, update copyright headers > - Merge > - Merge > - Initial commit Looks fine. I think dropping the `transferTo` overloads for now is good. - Marked as reviewed by bpb (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18598#pullrequestreview-2015549163
Re: RFR: 8329593: Drop adjustments to target parallelism when virtual threads do I/O on files opened for buffered I/O [v2]
> This change drops the adjustments to the virtual thread scheduler's target > parallelism when virtual threads do file operations on files that are opened > for buffered I/O. These operations are usually just too short to have any > benefit and may have a negative benefit when reading/writing a small number > of bytes. There is no change for read/write operations on files opened for > direct I/O or when writing to files that are opened with options for > synchronized I/O file integrity (O_SYNC/O_DSYNC and equivalents). Sergery > Kuksenko is polishing benchmarks that includes this area, this is for a > future PR. > > In addition, the blocker mechanism is updated to handle reentrancy (as can > happen if debugging code is added to ForkJoinPool) and preemption when > compensating (as can happen when substituting a heap buffer with a direct > buffer in some I/O operations). This part is a pre-requisite to the changes > to better support object monitor there are more places where preemption is > possible and this quickly leads to unbalanced begin/end. > > The changes have been baking in loom repo for some 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: - Merge - Sync up from loom repo, update copyright headers - Merge - Merge - Initial commit - Changes: - all: https://git.openjdk.org/jdk/pull/18598/files - new: https://git.openjdk.org/jdk/pull/18598/files/a21a8d6b..0d99fe0c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18598=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18598=00-01 Stats: 35845 lines in 976 files changed: 19000 ins; 10527 del; 6318 mod Patch: https://git.openjdk.org/jdk/pull/18598.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18598/head:pull/18598 PR: https://git.openjdk.org/jdk/pull/18598
Re: RFR: 8329593: Drop adjustments to target parallelism when virtual threads do I/O on files opened for buffered I/O
On Wed, 3 Apr 2024 10:04:36 GMT, Alan Bateman wrote: > This change drops the adjustments to the virtual thread scheduler's target > parallelism when virtual threads do file operations on files that are opened > for buffered I/O. These operations are usually just too short to have any > benefit and may have a negative benefit when reading/writing a small number > of bytes. There is no change for read/write operations on files opened for > direct I/O or when writing to files that are opened with options for > synchronized I/O file integrity (O_SYNC/O_DSYNC and equivalents). Sergery > Kuksenko is polishing benchmarks that includes this area, this is for a > future PR. > > In addition, the blocker mechanism is updated to handle reentrancy (as can > happen if debugging code is added to ForkJoinPool) and preemption when > compensating (as can happen when substituting a heap buffer with a direct > buffer in some I/O operations). This part is a pre-requisite to the changes > to better support object monitor there are more places where preemption is > possible and this quickly leads to unbalanced begin/end. > > The changes have been baking in loom repo for some time. The changes are updated to pin when attempting to increase parallelism. That removes the possibility of preemption in begingBlocking for now so hopefully this is easier to understand. Also removed the transferTo overloads for now, that would make it easier too but we may want to come back to them in the future. - PR Comment: https://git.openjdk.org/jdk/pull/18598#issuecomment-2069298348
Re: RFR: 8329593: Drop adjustments to target parallelism when virtual threads do I/O on files opened for buffered I/O
On Wed, 10 Apr 2024 00:41:39 GMT, Brian Burkhalter wrote: > changes look to be the most complicated here but I don't see any problems. I have some changes come that should make this easier to read, I'll update the PR in a few days. - PR Comment: https://git.openjdk.org/jdk/pull/18598#issuecomment-2046613420
Re: RFR: 8329593: Drop adjustments to target parallelism when virtual threads do I/O on files opened for buffered I/O
On Wed, 3 Apr 2024 10:04:36 GMT, Alan Bateman wrote: > This change drops the adjustments to the virtual thread scheduler's target > parallelism when virtual threads do file operations on files that are opened > for buffered I/O. These operations are usually just too short to have any > benefit and may have a negative benefit when reading/writing a small number > of bytes. There is no change for read/write operations on files opened for > direct I/O or when writing to files that are opened with options for > synchronized I/O file integrity (O_SYNC/O_DSYNC and equivalents). Sergery > Kuksenko is polishing benchmarks that includes this area, this is for a > future PR. > > In addition, the blocker mechanism is updated to handle reentrancy as can > happen if debugging code is added to ForkJoinPool, if there is preemption > when attempting to compensate, or potentially forced preemption in the > future. This part is a pre-requisite to the changes to better support object > monitor there are more places where preemption is possible and this quickly > leads to unbalanced begin/end. > > The changes have been baking in the loom repo for several months. The `CarrierThread` changes look to be the most complicated here but I don't see any problems. Otherwise, I don't have any comments aside from some that @jaikiran already made hence not worth repeating. - Marked as reviewed by bpb (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18598#pullrequestreview-1990512527
Re: RFR: 8329593: Drop adjustments to target parallelism when virtual threads do I/O on files opened for buffered I/O
On Tue, 9 Apr 2024 15:15:52 GMT, Jaikiran Pai wrote:
>> This change drops the adjustments to the virtual thread scheduler's target
>> parallelism when virtual threads do file operations on files that are opened
>> for buffered I/O. These operations are usually just too short to have any
>> benefit and may have a negative benefit when reading/writing a small number
>> of bytes. There is no change for read/write operations on files opened for
>> direct I/O or when writing to files that are opened with options for
>> synchronized I/O file integrity (O_SYNC/O_DSYNC and equivalents). Sergery
>> Kuksenko is polishing benchmarks that includes this area, this is for a
>> future PR.
>>
>> In addition, the blocker mechanism is updated to handle reentrancy as can
>> happen if debugging code is added to ForkJoinPool, if there is preemption
>> when attempting to compensate, or potentially forced preemption in the
>> future. This part is a pre-requisite to the changes to better support object
>> monitor there are more places where preemption is possible and this quickly
>> leads to unbalanced begin/end.
>>
>> The changes have been baking in the loom repo for several months.
>
> src/java.base/share/classes/jdk/internal/misc/CarrierThread.java line 81:
>
>> 79: value = ForkJoinPools.beginCompensatedBlock(getPool());
>> 80: } catch (Throwable e) {
>> 81: if (compensating == COMPENSATE_IN_PROGRESS) {
>
> I don't fully follow these checks `if (compensating ==
> COMPENSATE_IN_PROGRESS) {`. Surely it's not for concurrent access (given the
> `assert` at the start of this method, this code path happens in a single
> thread). So I think these checks are about the re-entrancy that is mentioned
> in the description of this PR.
> In the context of re-entrancy, if I am reading the code correctly, I don't
> see how a re-entrant call would end up on this line (and other similar lines)
> in this top level `if` block. When a thread enters the top level `if` block
> it immediately sets the `compensating` to `COMPENSATE_IN_PROGRESS`:
>
>
> if (compensating == NOT_COMPENSATING) {
> compensating = COMPENSATE_IN_PROGRESS;
> ...
>
> So a subsequent re-entrant call would never enter that top level `if` block
> again. Which leads me to question the need of these additional `if
> (compensating == COMPENSATE_IN_PROGRESS) {` checks. I feel like I am missing
> something in this code.
> In the context of re-entrancy, if I am reading the code correctly, I don't
> see how a re-entrant call would end up on this line (and other similar lines)
> in this top level if block. When a thread enters the top level if block it
> immediately sets the compensating to COMPENSATE_IN_PROGRESS: I feel like I am
> missing something in this code.
These are fields on the carrier. If a virtual thread is preempted when
compensating (or in the progress of) then it may continue on a different
carrier or the original carrier may execute the task for a different virtual
thread that does I/O. These are the cases that are handled here.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18598#discussion_r1557950295
Re: RFR: 8329593: Drop adjustments to target parallelism when virtual threads do I/O on files opened for buffered I/O
On Tue, 9 Apr 2024 14:50:00 GMT, Alan Bateman wrote: > It's to increase the target parallelism for the duration of the transfer > rather than specific read operations. Do you think we would need to do something similar in`PipeInputStream` that belongs to `Process`? - PR Review Comment: https://git.openjdk.org/jdk/pull/18598#discussion_r1557842319
Re: RFR: 8329593: Drop adjustments to target parallelism when virtual threads do I/O on files opened for buffered I/O
On Wed, 3 Apr 2024 10:04:36 GMT, Alan Bateman wrote:
> This change drops the adjustments to the virtual thread scheduler's target
> parallelism when virtual threads do file operations on files that are opened
> for buffered I/O. These operations are usually just too short to have any
> benefit and may have a negative benefit when reading/writing a small number
> of bytes. There is no change for read/write operations on files opened for
> direct I/O or when writing to files that are opened with options for
> synchronized I/O file integrity (O_SYNC/O_DSYNC and equivalents). Sergery
> Kuksenko is polishing benchmarks that includes this area, this is for a
> future PR.
>
> In addition, the blocker mechanism is updated to handle reentrancy as can
> happen if debugging code is added to ForkJoinPool, if there is preemption
> when attempting to compensate, or potentially forced preemption in the
> future. This part is a pre-requisite to the changes to better support object
> monitor there are more places where preemption is possible and this quickly
> leads to unbalanced begin/end.
>
> The changes have been baking in the loom repo for several months.
src/java.base/share/classes/jdk/internal/misc/CarrierThread.java line 81:
> 79: value = ForkJoinPools.beginCompensatedBlock(getPool());
> 80: } catch (Throwable e) {
> 81: if (compensating == COMPENSATE_IN_PROGRESS) {
I don't fully follow these checks `if (compensating == COMPENSATE_IN_PROGRESS)
{`. Surely it's not for concurrent access (given the `assert` at the start of
this method, this code path happens in a single thread). So I think these
checks are about the re-entrancy that is mentioned in the description of this
PR.
In the context of re-entrancy, if I am reading the code correctly, I don't see
how a re-entrant call would end up on this line (and other similar lines) in
this top level `if` block. When a thread enters the top level `if` block it
immediately sets the `compensating` to `COMPENSATE_IN_PROGRESS`:
if (compensating == NOT_COMPENSATING) {
compensating = COMPENSATE_IN_PROGRESS;
...
So a subsequent re-entrant call would never enter that top level `if` block
again. Which leads me to question the need of these additional `if
(compensating == COMPENSATE_IN_PROGRESS) {` checks. I feel like I am missing
something in this code.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18598#discussion_r1557834582
Re: RFR: 8329593: Drop adjustments to target parallelism when virtual threads do I/O on files opened for buffered I/O
On Tue, 9 Apr 2024 14:59:00 GMT, Jaikiran Pai wrote: > Was the use the word `tryCompensate` intentional here? I don't see that > method in this class or in the implementation of > `CarrierThread.beginBlocking()` The FJP method is named tryCompensate and the same method name was used here at one point. - PR Review Comment: https://git.openjdk.org/jdk/pull/18598#discussion_r1557814857
Re: RFR: 8329593: Drop adjustments to target parallelism when virtual threads do I/O on files opened for buffered I/O
On Wed, 3 Apr 2024 10:04:36 GMT, Alan Bateman wrote: > This change drops the adjustments to the virtual thread scheduler's target > parallelism when virtual threads do file operations on files that are opened > for buffered I/O. These operations are usually just too short to have any > benefit and may have a negative benefit when reading/writing a small number > of bytes. What I understand of these changes overall is that, we no longer consider most of the file operations that work against an actual file as needing to be compensated by the ForkJoinPool. It doesn't matter even if the read() is being done with a large buffer - we still consider it an operation that doesn't deserve a compensation. The only file operations where we do appear to compensate is certain `write` operations which are backed by `O_SYNC`, `O_DSYNC` or direct IO writes. The places where `FileOutputStream` is used against a file descriptor which isn't a file have been update to continue to using the `Blocker` to potentially compensate the operations. Overall, based on my limited knowledge of this area, the changes look OK to me. There are a few questions I had which I've added inline. The copyright years on many of the files will need an update. - PR Comment: https://git.openjdk.org/jdk/pull/18598#issuecomment-2045417779
Re: RFR: 8329593: Drop adjustments to target parallelism when virtual threads do I/O on files opened for buffered I/O
On Wed, 3 Apr 2024 10:04:36 GMT, Alan Bateman wrote: > This change drops the adjustments to the virtual thread scheduler's target > parallelism when virtual threads do file operations on files that are opened > for buffered I/O. These operations are usually just too short to have any > benefit and may have a negative benefit when reading/writing a small number > of bytes. There is no change for read/write operations on files opened for > direct I/O or when writing to files that are opened with options for > synchronized I/O file integrity (O_SYNC/O_DSYNC and equivalents). Sergery > Kuksenko is polishing benchmarks that includes this area, this is for a > future PR. > > In addition, the blocker mechanism is updated to handle reentrancy as can > happen if debugging code is added to ForkJoinPool, if there is preemption > when attempting to compensate, or potentially forced preemption in the > future. This part is a pre-requisite to the changes to better support object > monitor there are more places where preemption is possible and this quickly > leads to unbalanced begin/end. > > The changes have been baking in the loom repo for several months. src/java.base/share/classes/jdk/internal/misc/Blocker.java line 79: > 77: * Marks the beginning of a possibly blocking operation. > 78: * @param blocking true if the operation may block, otherwise false > 79: * @return true if tryCompensate attempted Was the use the word `tryCompensate` intentional here? I don't see that method in this class or in the implementation of `CarrierThread.beginBlocking()` - PR Review Comment: https://git.openjdk.org/jdk/pull/18598#discussion_r1557805449
Re: RFR: 8329593: Drop adjustments to target parallelism when virtual threads do I/O on files opened for buffered I/O
On Tue, 9 Apr 2024 13:58:31 GMT, Jaikiran Pai wrote:
>> This change drops the adjustments to the virtual thread scheduler's target
>> parallelism when virtual threads do file operations on files that are opened
>> for buffered I/O. These operations are usually just too short to have any
>> benefit and may have a negative benefit when reading/writing a small number
>> of bytes. There is no change for read/write operations on files opened for
>> direct I/O or when writing to files that are opened with options for
>> synchronized I/O file integrity (O_SYNC/O_DSYNC and equivalents). Sergery
>> Kuksenko is polishing benchmarks that includes this area, this is for a
>> future PR.
>>
>> In addition, the blocker mechanism is updated to handle reentrancy as can
>> happen if debugging code is added to ForkJoinPool, if there is preemption
>> when attempting to compensate, or potentially forced preemption in the
>> future. This part is a pre-requisite to the changes to better support object
>> monitor there are more places where preemption is possible and this quickly
>> leads to unbalanced begin/end.
>>
>> The changes have been baking in the loom repo for several months.
>
> src/java.base/share/classes/java/lang/System.java line 2262:
>
>> 2260: @Override
>> 2261: public long transferTo(OutputStream out) throws IOException {
>> 2262: boolean attempted = Blocker.begin();
>
> Hello Alan, why do we mark transferTo as potentially blocking operation which
> might need compensation? As far as I can see in the underlying implementation
> of `FileInputStream.transferTo()` it either calls the
> `FileChannel.transferTo()` or does a `in.read()` followed by `out.write()`.
> In either of those cases wouldn't the underlying `InputStream/OutputStream`
> already have the necessary `Blocker` calls?
It's to increase the target parallelism for the duration of the transfer rather
than specific read operations. System.in.transferTo(out) should be rare so it's
not super important of course.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18598#discussion_r1557786640
Re: RFR: 8329593: Drop adjustments to target parallelism when virtual threads do I/O on files opened for buffered I/O
On Tue, 9 Apr 2024 13:53:11 GMT, Jaikiran Pai wrote:
>> This change drops the adjustments to the virtual thread scheduler's target
>> parallelism when virtual threads do file operations on files that are opened
>> for buffered I/O. These operations are usually just too short to have any
>> benefit and may have a negative benefit when reading/writing a small number
>> of bytes. There is no change for read/write operations on files opened for
>> direct I/O or when writing to files that are opened with options for
>> synchronized I/O file integrity (O_SYNC/O_DSYNC and equivalents). Sergery
>> Kuksenko is polishing benchmarks that includes this area, this is for a
>> future PR.
>>
>> In addition, the blocker mechanism is updated to handle reentrancy as can
>> happen if debugging code is added to ForkJoinPool, if there is preemption
>> when attempting to compensate, or potentially forced preemption in the
>> future. This part is a pre-requisite to the changes to better support object
>> monitor there are more places where preemption is possible and this quickly
>> leads to unbalanced begin/end.
>>
>> The changes have been baking in the loom repo for several months.
>
> src/java.base/share/classes/java/lang/System.java line 2279:
>
>> 2277: }
>> 2278:
>> 2279: public void write(int b) throws IOException {
>
> Nit - missing an `@Override`
Okay.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18598#discussion_r1557769608
Re: RFR: 8329593: Drop adjustments to target parallelism when virtual threads do I/O on files opened for buffered I/O
On Tue, 9 Apr 2024 14:20:51 GMT, Jaikiran Pai wrote:
>> This change drops the adjustments to the virtual thread scheduler's target
>> parallelism when virtual threads do file operations on files that are opened
>> for buffered I/O. These operations are usually just too short to have any
>> benefit and may have a negative benefit when reading/writing a small number
>> of bytes. There is no change for read/write operations on files opened for
>> direct I/O or when writing to files that are opened with options for
>> synchronized I/O file integrity (O_SYNC/O_DSYNC and equivalents). Sergery
>> Kuksenko is polishing benchmarks that includes this area, this is for a
>> future PR.
>>
>> In addition, the blocker mechanism is updated to handle reentrancy as can
>> happen if debugging code is added to ForkJoinPool, if there is preemption
>> when attempting to compensate, or potentially forced preemption in the
>> future. This part is a pre-requisite to the changes to better support object
>> monitor there are more places where preemption is possible and this quickly
>> leads to unbalanced begin/end.
>>
>> The changes have been baking in the loom repo for several months.
>
> src/java.base/share/classes/sun/nio/ch/FileChannelImpl.java line 304:
>
>> 302: return 0;
>> 303: do {
>> 304: boolean attempted = Blocker.begin(sync | direct);
>
> Is the use of the bitwise operator `|` instead of `||` on `boolean` fields,
> here and other places, intentional?
Well spotted, these should be ||.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18598#discussion_r1557761764
Re: RFR: 8329593: Drop adjustments to target parallelism when virtual threads do I/O on files opened for buffered I/O
On Wed, 3 Apr 2024 10:04:36 GMT, Alan Bateman wrote:
> This change drops the adjustments to the virtual thread scheduler's target
> parallelism when virtual threads do file operations on files that are opened
> for buffered I/O. These operations are usually just too short to have any
> benefit and may have a negative benefit when reading/writing a small number
> of bytes. There is no change for read/write operations on files opened for
> direct I/O or when writing to files that are opened with options for
> synchronized I/O file integrity (O_SYNC/O_DSYNC and equivalents). Sergery
> Kuksenko is polishing benchmarks that includes this area, this is for a
> future PR.
>
> In addition, the blocker mechanism is updated to handle reentrancy as can
> happen if debugging code is added to ForkJoinPool, if there is preemption
> when attempting to compensate, or potentially forced preemption in the
> future. This part is a pre-requisite to the changes to better support object
> monitor there are more places where preemption is possible and this quickly
> leads to unbalanced begin/end.
>
> The changes have been baking in the loom repo for several months.
src/java.base/share/classes/sun/nio/ch/FileChannelImpl.java line 304:
> 302: return 0;
> 303: do {
> 304: boolean attempted = Blocker.begin(sync | direct);
Is the use of the bitwise operator `|` instead of `||` on `boolean` fields,
here and other places, intentional?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18598#discussion_r1557727112
Re: RFR: 8329593: Drop adjustments to target parallelism when virtual threads do I/O on files opened for buffered I/O
On Wed, 3 Apr 2024 10:04:36 GMT, Alan Bateman wrote:
> This change drops the adjustments to the virtual thread scheduler's target
> parallelism when virtual threads do file operations on files that are opened
> for buffered I/O. These operations are usually just too short to have any
> benefit and may have a negative benefit when reading/writing a small number
> of bytes. There is no change for read/write operations on files opened for
> direct I/O or when writing to files that are opened with options for
> synchronized I/O file integrity (O_SYNC/O_DSYNC and equivalents). Sergery
> Kuksenko is polishing benchmarks that includes this area, this is for a
> future PR.
>
> In addition, the blocker mechanism is updated to handle reentrancy as can
> happen if debugging code is added to ForkJoinPool, if there is preemption
> when attempting to compensate, or potentially forced preemption in the
> future. This part is a pre-requisite to the changes to better support object
> monitor there are more places where preemption is possible and this quickly
> leads to unbalanced begin/end.
>
> The changes have been baking in the loom repo for several months.
src/java.base/share/classes/jdk/internal/misc/CarrierThread.java line 104:
> 102: if (compensating == COMPENSATING) {
> 103: ForkJoinPools.endCompensatedBlock(getPool(),
> compensateValue);
> 104: compensating = NOT_COMPENSATING;
For the sake for consistency between the state and the `compensateValue`, would
it be good to reset `compensateValue` to `0` when we switch to
`NOT_COMPENSATING`?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18598#discussion_r1557714608
Re: RFR: 8329593: Drop adjustments to target parallelism when virtual threads do I/O on files opened for buffered I/O
On Wed, 3 Apr 2024 10:04:36 GMT, Alan Bateman wrote:
> This change drops the adjustments to the virtual thread scheduler's target
> parallelism when virtual threads do file operations on files that are opened
> for buffered I/O. These operations are usually just too short to have any
> benefit and may have a negative benefit when reading/writing a small number
> of bytes. There is no change for read/write operations on files opened for
> direct I/O or when writing to files that are opened with options for
> synchronized I/O file integrity (O_SYNC/O_DSYNC and equivalents). Sergery
> Kuksenko is polishing benchmarks that includes this area, this is for a
> future PR.
>
> In addition, the blocker mechanism is updated to handle reentrancy as can
> happen if debugging code is added to ForkJoinPool, if there is preemption
> when attempting to compensate, or potentially forced preemption in the
> future. This part is a pre-requisite to the changes to better support object
> monitor there are more places where preemption is possible and this quickly
> leads to unbalanced begin/end.
>
> The changes have been baking in the loom repo for several months.
src/java.base/share/classes/java/lang/System.java line 2262:
> 2260: @Override
> 2261: public long transferTo(OutputStream out) throws IOException {
> 2262: boolean attempted = Blocker.begin();
Hello Alan, why do we mark transferTo as potentially blocking operation which
might need compensation? As far as I can see in the underlying implementation
of `FileInputStream.transferTo()` it either calls the
`FileChannel.transferTo()` or does a `in.read()` followed by `out.write()`. In
either of those cases wouldn't the underlying `InputStream/OutputStream`
already have the necessary `Blocker` calls?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18598#discussion_r1557679570
Re: RFR: 8329593: Drop adjustments to target parallelism when virtual threads do I/O on files opened for buffered I/O
On Wed, 3 Apr 2024 10:04:36 GMT, Alan Bateman wrote:
> This change drops the adjustments to the virtual thread scheduler's target
> parallelism when virtual threads do file operations on files that are opened
> for buffered I/O. These operations are usually just too short to have any
> benefit and may have a negative benefit when reading/writing a small number
> of bytes. There is no change for read/write operations on files opened for
> direct I/O or when writing to files that are opened with options for
> synchronized I/O file integrity (O_SYNC/O_DSYNC and equivalents). Sergery
> Kuksenko is polishing benchmarks that includes this area, this is for a
> future PR.
>
> In addition, the blocker mechanism is updated to handle reentrancy as can
> happen if debugging code is added to ForkJoinPool, if there is preemption
> when attempting to compensate, or potentially forced preemption in the
> future. This part is a pre-requisite to the changes to better support object
> monitor there are more places where preemption is possible and this quickly
> leads to unbalanced begin/end.
>
> The changes have been baking in the loom repo for several months.
src/java.base/share/classes/java/lang/System.java line 2279:
> 2277: }
> 2278:
> 2279: public void write(int b) throws IOException {
Nit - missing an `@Override`
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18598#discussion_r1557670428
