RFR(L): 8195097: Make it possible to process StringTable outside safepoint
Robbin Ehn
robbin.ehn at oracle.com
Thu Jun 14 05:20:57 UTC 2018
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
>>>
More information about the hotspot-gc-dev
mailing list