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

[David Holmes]

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