RFR(L): 8195097: Make it possible to process StringTable outside safepoint

[Robbin Ehn]

Hi David, thanks noting.

Yes I'm aware of that, I included that fix in:
8204613: StringTable: Calculates wrong number of uncleaned items.
Which is now reviewed and about to be pushed.

/Robbin

On 2018-06-14 06:59, David Holmes wrote:
> Hi Robbin and reviewers,
> 
> On 5/06/2018 1:15 AM, Robbin Ehn wrote:
>> Hi Jiangli,
>>
>> On 2018-05-30 21:47, Jiangli Zhou wrote:
>>> Hi Robbin,
>>>
>>> I mainly focused on the archived string part during review. Here are my 
>>> comments, which are minor issues mostly.
>>>
>>> - stringTable.hpp
>>>
>>> Archived string is only supported with INCLUDE_CDS_JAVA_HEAP. Please add 
>>> NOT_CDS_JAVA_HEAP_RETURN_(NULL) for lookup_shared() and 
>>> create_archived_string() below so their call sites are handled properly when 
>>> java heap object archiving is not supported.
>>>
>>>   153   oop lookup_shared(jchar* name, int len, unsigned int hash);
>>>   154   static oop create_archived_string(oop s, Thread* THREAD);
>>>
>>
>> Fixed
> 
> create_archived_string() was not fixed.
> 
> https://bugs.openjdk.java.net/browse/JDK-8205002
> 
> Cheers,
> David
> 
>>> - stringTable.cpp
>>>
>>> How about renaming CopyArchive to CopyToArchive, so it’s more descriptive?
>>
>> Fixed
>>
>>>
>>> Looks like the ‘bool’ return type is not needed since we always return with 
>>> true and the result is not checked. How able changing it to return ‘void’?
>>>
>>>   774   bool operator()(WeakHandle<vm_string_table_data>* val) {
>>>
>>
>> The scanning done by the hashtable is stopped if we return false here.
>> So the return value is used by the hashtable to know if it should continue the 
>> walk over all items.
>>
>>>
>>> - genCollectedHeap.cpp
>>>
>>> Based on the assert at line 863, looks like ‘par_state_string’ is not NULL 
>>> when 'scope->n_threads() > 1’. Maybe the if condition at line 865 could be 
>>> simplified to be just ‘if (scope->n_threads() > 1)’?
>>>
>>>   862   // Either we should be single threaded or have a ParState
>>>   863   assert((scope->n_threads() <= 1) || par_state_string != NULL, 
>>> "Parallel but not ParState");
>>>   864
>>>   865   if (scope->n_threads() > 1 && par_state_string != NULL) {
>>
>> Fixed, I'll send new version to rfr mail.
>>
>> Thanks, Robbin
>>
>>>
>>>
>>> Thanks,
>>> Jiangli
>>>
>>>> On May 28, 2018, at 6:19 AM, Robbin Ehn <robbin.ehn at oracle.com> wrote:
>>>>
>>>> Hi all, please review.
>>>>
>>>> This implements the StringTable with the ConcurrentHashtable for managing the
>>>> strings using oopStorage for backing the actual oops via WeakHandles.
>>>>
>>>> The unlinking and freeing of hashtable nodes is moved outside the safepoint,
>>>> which means GC only needs to walk the oopStorage, either concurrently or in a
>>>> safepoint. Walking oopStorage is also faster so there is a good effect on all
>>>> safepoints visiting the oops.
>>>>
>>>> The unlinking and freeing happens during inserts when dead weak oops are
>>>> encountered in that bucket. In any normal workload the stringtable self-cleans
>>>> without needing any additional cleaning. Cleaning/unlinking can also be done
>>>> concurrently via the ServiceThread, it is started when we have a high ‘dead
>>>> factor’. E.g. application have a lot of interned string removes the references
>>>> and never interns again. The ServiceThread also concurrently grows the table if
>>>> ‘load factor’ is high. Both the cleaning and growing take care to not 
>>>> prolonging
>>>> time to safepoint, at the cost of some speed.
>>>>
>>>> Kitchensink24h, multiple tier1-5 with no issue that I can relate to this
>>>> changeset, various benchmark such as JMH, specJBB2015.
>>>>
>>>> Issue: https://bugs.openjdk.java.net/browse/JDK-8195097
>>>> Webrev: http://cr.openjdk.java.net/~rehn/8195097/v0/webrev/
>>>>
>>>> Thanks, Robbin
>>>