On Tue, 9 Apr 2024 15:15:52 GMT, Jaikiran Pai <[email protected]> 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
