RFR: 8306738: Select num workers for safepoint ParallelCleanupTask [v3]

[Aleksey Shipilev]

On Mon, 8 May 2023 08:42:45 GMT, Axel Boldt-Christmas <aboldtch at openjdk.org> wrote:

>> src/hotspot/share/classfile/stringTable.cpp line 538:
>> 
>>> 536: }
>>> 537: 
>>> 538: uint StringTable::rehash_table_expected_workers() {
>> 
>> We always reply either `0` or `1` from these. So logically, it seems to answer whether we need heavy work. So, this method is then simpler, and matching the structure of the relevant `rehash_table`:
>> 
>> 
>> // Quick checks if we would do any heavy-weight safepoint work in rehash_table.
>> // Add shortcut cases here when changing rehash_table.
>> bool StringTable::needs_safepoint_rehash_table() {
>>   // Grow instead of rehash
>>   if (should_grow()) return false;
>> 
>>   // Already rehashed
>>   if (_rehashed) return false;
>> 
>>   // Should and can do alt-rehashing
>>   if (!_needs_rehashing) return false;
>>   if (!_local_table->is_safepoint_safe()) return false;
>> 
>>   return true;
>> }
>
> I prefer using a boolean return value as well. I just had trouble with coming up with a non confusing name for the method. Maybe `needs_safepoint_rehash_table` is good enough.
> 
> I'll rewrite the condition with multiple ifs. 
> 
> My knowledge of the concurrent hash table is rather limited, but what does the `Should and can do alt-rehashing` comment mean? AFAIK the `_local_table->is_safepoint_safe()` check is just to see if someone is currently resizing the table when we entered the safepoint.

Maybe "Should and can do alt-rehashing" is too strong indeed. "Need and can do rehashing" is more neutral here, and matches what code is actually checking.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/13616#discussion_r1187196827