RFR: 8315559: Delay TempSymbol cleanup to avoid symbol table churn [v3]

[Coleen Phillimore]

On Mon, 30 Oct 2023 16:59:45 GMT, Oli Gillespie <ogillespie at openjdk.org> wrote:

>> Attempt to fix regressions in class-loading performance caused by fixing a symbol table leak in [JDK-8313678](https://bugs.openjdk.org/browse/JDK-8313678).
>> 
>> See lengthy discussion in https://bugs.openjdk.org/browse/JDK-8315559 for more background. In short, the leak was providing an accidental cache for temporary symbols, allowing reuse.
>> 
>> This change keeps new temporary symbols alive in a queue for a short time, allowing them to be re-used by subsequent operations. For example, when attempting to load a class we may call JVM_FindLoadedClass for multiple classloaders back-to-back, and all of them will create a TempNewSymbol for the same string. At present, each call will leave a dead symbol in the table and create a new one. Dead symbols add cleanup and lookup overhead, and insertion is also costly. With this change, the symbol from the first invocation will live for some time after it is used, and subsequent callers can find the symbol alive in the table - avoiding the extra work.
>> 
>> The queue is bounded, and when full new entries displace the oldest entry. This means symbols are held for the time it takes for 100 new temp symbols to be created. 100 is chosen arbitrarily - the tradeoff is memory usage versus 'cache' hit rate.
>> 
>> When concurrent symbol table cleanup runs, it also drains the queue.
>> 
>> In my testing, this brings Dacapo pmd performance back to where it was before the leak was fixed.
>> 
>> Thanks @shipilev , @coleenp and @MDBijman for helping with this fix.
>
> Oli Gillespie has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix failing tests
>   
>   TempNewSymbol now increments refcount again, messing with the
>   expectations. This is not a complete fix, I will have to read the
>   individual cases and make sure they are correct.

I think this looks really good.  Test idea enclosed.

src/hotspot/share/oops/symbolHandle.hpp line 86:

> 84:     // or entries that are held elsewhere - it's a waste of effort.
> 85:     if (s != nullptr && s->refcount() == 1) {
> 86:       add_to_cleanup_delay_queue(s);

This could add the comment that adding to the delay queue will increment the refcount again for the entry while in the queue (had to look again to verify that).

src/hotspot/share/oops/symbolHandle.hpp line 114:

> 112:   // and this queue allows them to be reused instead of churning.
> 113:   void add_to_cleanup_delay_queue(Symbol* sym) {
> 114:     if (sym == nullptr) return;

sym is never null here since you check it above.

test/hotspot/gtest/classfile/test_symbolTable.cpp line 40:

> 38:   int abccount = abc->refcount();
> 39:   TempNewSymbol ss = abc;
> 40:   // TODO: properly account for Symbol cleanup delay queue

I wonder if you can programmatically change the queue length to zero and keep these counts.

Then add a test with some loop of n being the queue length, and create n symbols and check that the first has been reclaimed?

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

PR Review: https://git.openjdk.org/jdk/pull/16398#pullrequestreview-1705095684
PR Review Comment: https://git.openjdk.org/jdk/pull/16398#discussion_r1376819360
PR Review Comment: https://git.openjdk.org/jdk/pull/16398#discussion_r1376816877
PR Review Comment: https://git.openjdk.org/jdk/pull/16398#discussion_r1376815796