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

[Oli Gillespie]

On Wed, 1 Nov 2023 15:24:28 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

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

Thanks. Is the 'leak' is relevant in this case since we're shutting down anyway? I tried draining the queue in before_exit but that doesn't seem to run with ctrl-c/SIGINT shutdowns.

> HotSpot code avoids variables with static storage duration for types with
non-trivial construction or destruction

Do you have a suggested alternative? Everything in symbol table seems to be static.

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

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