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
