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

[Jie Kang]

On Fri, 4 Dec 2020 21:31:36 GMT, Markus Grönlund <mgronlun at openjdk.org> wrote:

>> I'm not sure either, Markus knows the epoch system the best, but I wouldn't change it.
>> 
>> We like to clear the cache (to reduce heap usage) when a physical recording ends, so I think your fix is fine for that reason alone. Possibly there should be a call to reset in PlatformRecorder::destroy() as well. It is however only called in the shutdown hook, so in practise it will probably not matter.
>
> Hi Jie,
> 
> I think what you have run into is something that I feel a bit guilty about...
> 
> So, it used to be the case that almost every subsystem (in the VM) maintained a local, i.e. to that subsystem, state. After introducing the epoch mechanism as the general "state progression", it was possible to consolidate almost all local states to use the general one. This works well, because all VM subsystems are notified / track when the epoch changes...
> 
> So this leads to the StringPool located in java...it used to be the case that the corresponding StringPool in native maintained a local state, a monotonically increasing integer value that was then read from the java side to detect when the system progressed. When i modified to use epoch, i did not think about the fact that the Java side does not get notified correctly, and a boolean will overflow quite quickly compared to the original integer...bahh
> 
> I think the correct way to fix this is to reinstate the original integer that increments on epoch shifts, and have that be read again from the java side, then there should not be overflowing happens that trick the system to thinking it is actually in the same epoch. This will avoid more cumbersome notification logic that would have to extend to java. It is also something that we had working before. 
> 
> With Loom, there will be a general counter incrementing tracking the epoch / generation state, so we could easily re-route the local StringPool state to the global one when Loom comes in...
> 
> Sorry for this inconvenience and thank you for finding this - i will suggest the code changes we could do to solve this.
> 
> Markus

Hi Markus,
Thanks for taking a look and sharing more context behind the current StringPool implementation. From your last statement: Is it okay for me to close this PR then and expect a future fix from you? Did I understand that correctly?

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

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