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

[Kim Barrett]

On Wed, 1 Nov 2023 12:15:35 GMT, Oli Gillespie <ogillespie at openjdk.org> wrote:

>> 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.
>
> A colleague suggested that was the appropriate constraint ("there is no transitive memory effects that ride on the counter"), but I don't understand it enough to defend it myself. Atomic::inc uses memory_order_conservative. Happy to go with whatever is preferred.

One of the things I'm worried about is that I think the decrement operation
might be able to become temporarily negative. I'm not sure whether that's a
problem. Maybe not. I was a little surprised that the queue length stuff is
signed rather than unsigned, as I expected the latter.

The idiom I've usually used for such things is increment-before-push and
decrement-after-pop. That ensures the counter never goes negative or
underflows.  Also usually use unsigned types for "sizes".

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

The asserts are there to catch memory leaks. It is the responsibility of the
owner of the NBQ to ensure it is empty before deleting it. (Probably that
should have been mentioned in a comment in the header.) This is a generic
requirement, and is not specific to G1DCQS (the sole previous client).

HotSpot code avoids variables with static storage duration for types with
non-trivial construction or destruction. (I thought this was discussed in the
style guide, but apparently not.  It's been discussed multiple times in
various reviews.)  That's what's happening here - there's no owning object for
the NBQ, and there's no explicit setup/teardown of it during VM initialization
and destruction.  That's the reason for hitting the asserts.

So no, don't add a configuration argument for NBQ.  I wouldn't approve that
either.

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

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