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

[Robbin Ehn]

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

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