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

[Jiangli Zhou]

Hi Robbin and David,

> On Jun 13, 2018, at 10:20 PM, Robbin Ehn <robbin.ehn at oracle.com> wrote:
> 
> 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.

Thanks for noticing that and fixing quickly. I didn’t catch it in the updated review.

Thanks!

Jiangli

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