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

[Oli Gillespie]

On Tue, 31 Oct 2023 18:01:03 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

>> 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
>
> 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.

Thanks! Yes, I agree that it's not perfectly bounded but that this usage works out okay.

> 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.

These assertions require the queue to be empty when the destructor is called. My static queue is destroyed at shutdown, and it may not be empty. I see no explicit problem with a queue not being empty when destroyed, I can only assume it was added to serve as an extra reminder for the specific use-case of g1dirtycardsetqueue which is the only existing user of NonblockingQueue. In my opinion it's better for individual queue owners to decide if they care about this.

Alternatives: I can modify NonblockingQueue to accept an argument (e.g. check_empty_at_shutdown) which is true for g1 card set and false for my usage. Or I can avoid NonblockingQueue entirely as there are a few other review concerns arising from its limitations.

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

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