On Wed, 4 Nov 2026 00:42:03 GMT, Erik Gahlin <[email protected]> wrote:
>> Could I have a review of a PR that fixes a bug in the JFR instrumentation of
>> RandomAccessFile?
>>
>> The problem is that readLine() may produce an event with non-zero number of
>> bytes read and EOF at the same time. To ensure that we get the correct
>> number of bytes, we can't just take the length of the returned String, since
>> readLine() may read one ('\n') or two bytes ('\r\n') for line endings.
>>
>> I added a test that verifies that readLine() returns the same result and
>> leaves the file pointer at the same position with or without JFR enabled.
>>
>> Testing: jdk-tier2 + test/jdk/jdk/jfr
>>
>> Thanks
>> Erik
>>
>>
>> ---------
>> - [x] I confirm that I make this contribution in accordance with the
>> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai).
>
> Erik Gahlin has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Use -1 to detect EOF
src/java.base/share/classes/java/io/RandomAccessFile.java line 1043:
> 1041: boolean endOfFile = false;
> 1042: try {
> 1043: while (!eol) {
The new code incurs JFR-related bookkeeping (i.e., `endOfFile` and `bytesRead`
updates) and `try`/`finally` costs even when `!jfrTracing`. Has it been
verified (via JMH?) that these extra costs when JFR is inactive are indeed
negligible? Presumably `RAF::readLine` can get very hot in a typical
application.
src/java.base/share/classes/java/io/RandomAccessFile.java line 1071:
> 1069: return null;
> 1070: }
> 1071: return input.toString();
Nit: Since `diff` is not preserved anyway, you might consider simplifying this:
return c == -1 && input.isEmpty() ? null : input.toString();
test/jdk/jdk/jfr/event/io/IOEvent.java line 122:
> 120: return new IOEvent(Thread.currentThread().getName(),
> EventType.FileForce, 0, address, false);
> 121: }
> 122: public static IOEvent createFileReadEvent(long size, File file,
> boolean endOfStream) {
Nit:
Suggestion:
public static IOEvent createFileReadEvent(long size, File file, boolean
endOfStream) {
test/jdk/jdk/jfr/event/io/TestRandomAccessFileReadLine.java line 37:
> 35:
> 36: /**
> 37: * @test
Suggestion:
/*
* @test
* @bug 8396523
test/jdk/jdk/jfr/event/io/TestRandomAccessFileReadLine.java line 41:
> 39: * @requires vm.hasJFR
> 40: * @library /test/lib /test/jdk
> 41: * @run main/othervm jdk.jfr.event.io.TestRandomAccessFileReadLine
Nit:
Suggestion:
* @run main/othervm ${test.main.class}
test/jdk/jdk/jfr/event/io/TestRandomAccessFileReadLine.java line 51:
> 49: System.out.println("Disabled: " + linesDisabled);
> 50: if (!linesEnabled.equals(linesDisabled)) {
> 51: throw new Exception("Expected same result from
> RandomAccessFile::readLine with or without JFR");
Nit: You may considering removing `println`'s and highlighting the problem only
on the unhappy path:
assertEquals(
linesEnabled, linesDisabled,
"Expected same result from RandomAccessFile::readLine with or without
JFR");
Either way, it is fine.
test/jdk/jdk/jfr/event/io/TestRandomAccessFileReadLine.java line 55:
> 53: }
> 54:
> 55: public static List<String> test(boolean enabled) throws Throwable {
I'm not really sure if this way of toggling JFR is sufficient:
1. `test(true)` already enables the JFR for the VM, hence `test(false)` runs in
a VM where `RandomAccessFile.jfrTracing` is `true`.
2. Even if you only have `test(false)`, `new Recording()` trickles down all the
way to `JFRTracing.enable()`.
That is, effectively, JFR (i.e., `RandomAccessFile.jfrTracing`) is always on
for this test.
You may consider multiple JTreg runs where you combine the result:
@run main/othervm ${test.main.class} testWithJfr
@run main/othervm ${test.main.class} testWithoutJfr
@run main/othervm ${test.main.class} verifyResults
Nit: `test()` can be `private`.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/33637#discussion_r4465413359
PR Review Comment: https://git.openjdk.org/jdk/pull/33637#discussion_r4465391614
PR Review Comment: https://git.openjdk.org/jdk/pull/33637#discussion_r4465586288
PR Review Comment: https://git.openjdk.org/jdk/pull/33637#discussion_r4465482765
PR Review Comment: https://git.openjdk.org/jdk/pull/33637#discussion_r4465606828
PR Review Comment: https://git.openjdk.org/jdk/pull/33637#discussion_r4465689858
PR Review Comment: https://git.openjdk.org/jdk/pull/33637#discussion_r4465991351
