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

[Oli Gillespie]

On Tue, 31 Oct 2023 18:53:18 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> 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.
>
> If you have to add more code to wrap NonblockingQueue, please implement it in a .cpp file.  I thought NBQ was sufficient for this.  Maybe we want some other data structure for this.

I have attempted to avoid locks if at all possible. Re ABA problems, my reading of the push/pop concern is that the problem occurs if a caller pops a node and then re-uses that same node later by re-pushing it. Something like this:

Queue contains Node1(val=a, next=null).
1. Thread 1 (start push) pushes Node2(val=b, next=null)
2. Thread 2 pops Node1(val=a, next=null)
3. Thread 2 sees that Node1.next is null and reuses the container, making Node1(val=c, next=null)
4. Thread 2 (start push) starts to push Node1(val=c, next=null)
5. Thread 1 (finish push) sets Node1.next=Node2
6. Thread 2 (finish push) sets Node2.next=Node1 <-- circular list

Since my change doesn't reuse nodes, I believe it's safe.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16398#discussion_r1378716964