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

[Kim Barrett]

On Wed, 1 Nov 2023 16:34:27 GMT, Oli Gillespie <ogillespie at openjdk.org> wrote:

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

I've filed a bug report against the style guide to provide guidance in this area.
https://bugs.openjdk.org/browse/JDK-8319242

We typically define a variable with a pointer type, and initialize it somewhere in the VM startup process.  So in
this case, something like

// declared in class
static TempSymbolDelayQueue* _cleanup_delay;

// in the .cpp
TempSymbolDelayQueue* SymbolHandleBase::_cleanup_delay = nullptr;
// and update uses accordingly.

Also declare and define an initialization function to initialize that variable, and call it somewhere in init.cpp,
probably in init_globals().  Use the style that exists there.  I expect if you don't put it early enough that you'll
reliably get an obvious crash from attempting to access a nullptr.

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

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