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

Mon Nov 13 01:42:09 UTC 2023

On Fri, 10 Nov 2023 12:23:27 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 indentation, rename test helper

Changes requested by kbarrett (Reviewer).

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

> 46: class SymbolHandleBase : public StackObj {
> 47:   static Symbol* volatile _cleanup_delay_queue[];
> 48:   static volatile uint _cleanup_delay_index;

Putting the delay queue implementation in SymbolHandleBase<> makes unnecessary and unused
data and possibly copies of the code. It is only used in the case where the template parameter is true.
Better would be to put the cleanup delay queue in a separate, non-templated, class. The entire
implementation of the queue could then be in the .cpp file. (I suggest the overhead of an out-of-line
call to add to the queue is in the noise, given that adding to the queue performs 3 atomic RMW
operations.)  So something like this:

// In symbolHandle.hpp.
class TempSymbolCleanupDelayer : AllStatic {
  // Or make these file-scoped statics in the .cpp file.
  static const uint QueueSize = 128;
  static Symbol* volatile _queue[];
  static volatile uint _index;

public:
  static void delay_cleanup(Symbol* s);
};

// In symbolHandle.cpp.
Symbol* volatile TempSymbolCleanupDelayer::_queue[QueueSize] = {};
volatile uint TempSymbolCleanupDelayer::_index = 0;

void TempSymbolCleanupDelayer::delay_cleanup(Symbol* sym) {
  assert(sym != nullptr, "precondition");
  sym->increment_refcount();
  uint i = Atomic::add(&_index, 1u) % QueueSize;
  Symbol* old = Atomic::xchg(&_queue, sym);
  Symbol::maybe_decrement_refcount(old);
}

Code is completely untested.  It incorporates suggestions I'm making elsewhere in this review too.

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

> 51: 
> 52: public:
> 53:   static constexpr uint CLEANUP_DELAY_MAX_ENTRIES = 128;

This doesn't need to be public.

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

> 56: 
> 57:   // Conversion from a Symbol* to a SymbolHandleBase.
> 58:   // Does not increment the current reference count if temporary.

This comment is no longer true for temp symbols, since adding to the delay queue increments the refcount.

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

> 57:   // Conversion from a Symbol* to a SymbolHandleBase.
> 58:   // Does not increment the current reference count if temporary.
> 59:   SymbolHandleBase(Symbol *s) : _temp(s) {

Is this really called with nullptr sometimes?  It would be better if that was disallowed.  But that's probably
outside the scope of this PR.

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

> 97:     sym->increment_refcount();
> 98:     STATIC_ASSERT(is_power_of_2(CLEANUP_DELAY_MAX_ENTRIES)); // allow modulo shortcut
> 99:     uint i = Atomic::add(&_cleanup_delay_index, 1u) & (CLEANUP_DELAY_MAX_ENTRIES - 1);

I disagree with @shipilev .  Just use `% CLEANUP_DELAY_MAX_ENTRIES`.  This is an entirely
unreasonable amount of code to explicitly provide a micro-optimization that any non-stupid compiler
will do for us anyway. At most, add a comment to the definition of that constant that being a power
of 2 benefits the ring-buffer.  But I wouldn't even bother with that.

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

> 101:     if (old != nullptr) {
> 102:       old->decrement_refcount();
> 103:     }

This conditional refcount decrement could instead be `Symbol::maybe_decrement_refcount(old);`.

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

> 113:   }
> 114: 
> 115:   static void drain_cleanup_delay_queue() {

It's not obvious that draining the queue is useful. Unless there's a reason I'm missing, I suggest not doing so.

test/hotspot/gtest/classfile/test_placeholders.cpp line 45:

> 43:   Symbol* D = SymbolTable::new_symbol("def2_8_2023_class");
> 44:   Symbol* super = SymbolTable::new_symbol("super2_8_2023_supername");
> 45:   Symbol* interf = SymbolTable::new_symbol("interface2_8_2023_supername");

This doesn't seem like the right way to update this test.  Doesn't this leave the symbols dangling?
And in the face of potential queue draining, it seems to me this could lead the test to intermittent failures.

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

PR Review: https://git.openjdk.org/jdk/pull/16398#pullrequestreview-1726414311
PR Review Comment: https://git.openjdk.org/jdk/pull/16398#discussion_r1390538976
PR Review Comment: https://git.openjdk.org/jdk/pull/16398#discussion_r1390539047
PR Review Comment: https://git.openjdk.org/jdk/pull/16398#discussion_r1390539239
PR Review Comment: https://git.openjdk.org/jdk/pull/16398#discussion_r1390542435
PR Review Comment: https://git.openjdk.org/jdk/pull/16398#discussion_r1390547204
PR Review Comment: https://git.openjdk.org/jdk/pull/16398#discussion_r1390548044
PR Review Comment: https://git.openjdk.org/jdk/pull/16398#discussion_r1390542654
PR Review Comment: https://git.openjdk.org/jdk/pull/16398#discussion_r1390549795