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

[Kim Barrett]

On Tue, 31 Oct 2023 12:27:53 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:
> 
>   Adress comments
>   
>   Fix indentation
>   Improve tests
>   Improve comment
>   Remove redundant null check
>   Improve naming
>   Pop when >, not >= max len

Changes requested by kbarrett (Reviewer).

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

> 119: 
> 120:     // If the queue is now full, implement a one-in, one-out policy.
> 121:     if (Atomic::add(&_cleanup_delay_len, 1, memory_order_relaxed) > _cleanup_delay_max_entries) {

Why is incrementing relaxed?  Now I have to think hard about whether there
might be any ordering problems resulting from that.

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

> 120:     // If the queue is now full, implement a one-in, one-out policy.
> 121:     if (Atomic::add(&_cleanup_delay_len, 1, memory_order_relaxed) > _cleanup_delay_max_entries) {
> 122:       TempSymbolDelayQueueNode* result = _cleanup_delay.pop();

NonblockingQueue's push and pop operations are subject to ABA problems, and
require the client to address that in some fashion. There's nothing here to do
that. I think one possibility would be to wrap the push/pop calls in a
GlobalCounter::CriticalSection and do a GlobalCounter::write_synchronize
before deleting a node.

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

> 123:       if (result != nullptr) {
> 124:         delete result;
> 125:         Atomic::dec(&_cleanup_delay_len);

Because of a limitation on NonblockingQueue (from the class description: "A
queue may appear empty even though elements have been added and not
removed."), it is theoretically possible for the max-entries value to be
exceeded.  (List is empty, thread1 starts a push but is paused, other threads
push lots of entries.)  But that will eventually be cleaned up by completion
of the initial push and then later draining the list.  So I don't think this
is a problem in practice, but wanted to note that I'd looked at the question.

src/hotspot/share/utilities/nonblockingQueue.inline.hpp line 51:

> 49:   assert(_tail == nullptr, "precondition");
> 50: }
> 51: #endif

Why is this being removed?  Without some good explanation, I'm disapproving this change.

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

PR Review: https://git.openjdk.org/jdk/pull/16398#pullrequestreview-1706902214
PR Review Comment: https://git.openjdk.org/jdk/pull/16398#discussion_r1377999358
PR Review Comment: https://git.openjdk.org/jdk/pull/16398#discussion_r1377998043
PR Review Comment: https://git.openjdk.org/jdk/pull/16398#discussion_r1377987280
PR Review Comment: https://git.openjdk.org/jdk/pull/16398#discussion_r1377955872