RFR: 8301992: Embed SymbolTable CHT node [v4]

[Ioi Lam]

On Thu, 16 Feb 2023 19:35:45 GMT, Calvin Cheung <ccheung at openjdk.org> wrote:

>> src/hotspot/share/classfile/symbolTable.cpp line 168:
>> 
>>> 166:       if (!SymbolTable::arena()->Afree(memory, alloc_size)) {
>>> 167:         log_trace_symboltable_helper(&value, "Leaked permanent symbol");
>>> 168:       }
>> 
>> We should avoid freeing if DumpSharedSpaces is true, so that the code is symmetrical with allocate_node_impl(). Maybe this:
>> 
>> 
>> #if INCLUDE_CDS
>>   if (DumpSharedSpaces) {
>>     // no deallocation is needed
>>   } else
>> #endif
>>   if (value.refcount() != PERM_REFCOUNT) {
>
> How about performing the above check at the beginning of the function, right after the assert statement as follows?
> Let me know if the added assert is necessary.
> 
> --- a/src/hotspot/share/classfile/symbolTable.cpp
> +++ b/src/hotspot/share/classfile/symbolTable.cpp
> @@ -151,6 +151,13 @@ public:
>      // If #2, then the symbol is dead (refcount==0)
>      assert(value.is_permanent() || (value.refcount() == 1) || (value.refcount() == 0),
>             "refcount %d", value.refcount());
> +#if INCLUDE_CDS
> +    if (DumpSharedSpaces) {
> +      assert(value.is_permanent(), "symbol should be permanent during CDS dumping");
> +      // no deallocation is needed
> +      return;
> +    }
> +#endif
>      if (value.refcount() == 1) {
>        value.decrement_refcount();
>        assert(value.refcount() == 0, "expected dead symbol");

Currently, in `allocate_node_impl()` there's no guarantee that the refcount is always permanent when DumpSharedSpaces is true.

During CDS dumping, some classes may be loaded by non-boot loaders, and their symbols may initially have a non-permanent count. Eventually, we force all the archived symbols to be permanent, but theoretically calls to `free_node()` may happen before that.

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

PR: https://git.openjdk.org/jdk/pull/12562