RFR: 8329593: Drop adjustments to target parallelism when virtual threads do I/O on files opened for buffered I/O

[Alan Bateman]

On Tue, 9 Apr 2024 15:15:52 GMT, Jaikiran Pai <jpai at openjdk.org> 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