RFR: 8257621: JFR StringPool misses cached items across consecutive recordings

[Jie Kang]

On Fri, 4 Dec 2020 15:19:57 GMT, Erik Gahlin <egahlin at openjdk.org> wrote:

>> This addresses 8257621 by resetting the Java cache when a recording is stopped and there are no other recordings still running. The reproducer described in 8257621 no longer exhibits the bug with this change. However, I'm not sure if there are other edge cases missed, or if this fits with the expected design of the StringPool, Recording and Chunk systems. Any insight would be appreciated; I'm willing to adjust as needed.
>> 
>> On:
>> Linux (Fedora 31), jtreg 5.1-b01 configured with flags --disable-warnings-as-errors --with-jtreg=/path/to/jtreg-5.1-b01/
>> 
>> I have run the following with this patch applied:
>> `make run-test TEST=":jdk_jfr"`
>> `make run-test-tier1`
>> 
>> The test `jdk.jfr.api.event.TestShouldCommit` fails for me but I believe it is fragile and not relevant to the change proposed here.
>
> This looks like the same issue as in https://bugs.openjdk.java.net/browse/JDK-8256482, which started to happen after the pool started to count at 1 instead of 0.
> 
> Do you know why the change of the pool id provoked the error? And if, or why, this fixes the problem?

@egahlin

Using the 'jfr' tool to examine the events and constant pools for jfr files produced, this bug JDK-8257621 `(consecutive recording)` exists on commit `c5fe2c1fcb5b5cec0a3f53716c24b2778467900b` which is one prior to my fix for JDK-8255992 `(id with 0)`. You will find with the reproducer I wrote in the description for `(consecutive recording)`, that the java.lang.String constant pool exists in the first recording and does not exist in the second recording.

Prior to the fix for `(id with 0)`, the first string to meet the StringPool cache requirement would result in a constant pool entry that was unused. The events would have the full string embedded in every instance, across consecutive recordings. After the fix, the events would have the id embedded which would then link to the constant pool entry. Thus, `(consecutive recording)` is more exposed after the fix for `(id with 0)` because the constant pool entry does not exist in consecutive recordings and the strings would not be resolvable. This can explain the failure seen in JDK-8256482.


This test did not fail on my machine because the StringPool is conditional on string length:
    static final int MIN_LIMIT = 16;
    static final int MAX_LIMIT = 128; /* 0 MAX means disabled */

My file system setup results in a test with Path strings of length ~185, and so the problematic code paths are not executed. It would be reassuring to hear if the test system paths were length within 16 and 128. Also, I think I will submit more test cases to the suite that will exercise the StringPool more explicitly when cycles allow.

-------------

PR: https://git.openjdk.java.net/jdk/pull/1576