RFR(L): 8195097: Make it possible to process StringTable outside safepoint
Jiangli Zhou
jiangli.zhou at oracle.com
Thu Jun 14 16:56:54 UTC 2018
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
>>>>
More information about the hotspot-gc-dev
mailing list