Review request: 8015422: Large performance hit when the StringTable is walked twice in Parallel Scavenge

[Stefan Karlsson]

On 05/28/2013 10:41 AM, Per Lidén wrote:
> Hi,
>
> On 2013-05-28 10:04, Stefan Karlsson wrote:
>> On 05/27/2013 08:43 PM, Stefan Karlsson wrote:
>>> This patch changes some Runtime code, so I've CC:ed 
>>> hotspot-runtime-dev.
>>>
>>> See inlined:
>>>
>>> On 2013-05-27 16:25, Per Lidén wrote:
>>>> Hi,
>>>>
>>>> On 2013-05-27 13:50, Stefan Karlsson wrote:
>>>>> http://cr.openjdk.java.net/~stefank/8015422/webrev.00/
>>>>>
>>>>> 8015422: Large performance hit when the StringTable is walked 
>>>>> twice in
>>>>> Parallel Scavenge
>>>>> Summary: Combine the calls to StringTable::unlink and
>>>>> StringTable::oops_do in Parallel Scavenge.
>>>>>
>>>>> The patch is built on top of this clean-up patch:
>>>>>   http://cr.openjdk.java.net/~stefank/8015428/webrev.00/
>>>>> http://mail.openjdk.java.net/pipermail/hotspot-dev/2013-May/009735.html 
>>>>>
>>>>>
>>>>> The fix has been verified to give ~10% lower young GC times on the 
>>>>> CRM
>>>>> Sales Opty, the benchmark where this issue was found.
>>>>
>>>> Nice! Looks good,
>>>
>>> Thanks.
>>>
>>>> just a minor naming convention question. Would it make sense to call
>>>> the new unlink() function oops_do() instead? I think of oops_do() as
>>>> "adjust or remove references"
>>>
>>> I prefer to think of oops_do() as functions that should be agnostic to
>>> the OopClosure passed in. The closure could mark, adjust, verify,
>>> print, etc.
>
> I think your right, oops_do() should probably be considered to be more 
> generic than my initial description.
>
>>>
>>>> and unlink() is to me more "nothing needs to be adjusted, just remove
>>>> any dead references".
>>>
>>> What about StringTable::clean(BoolObjectClosure* is_alive, OopClosure*
>>> f) ?
>>
>> Maybe:
>>   StringTable::unlink_or_apply(BoolObjectClosure* is_alive, 
>> OopClosure* f)
>>   StringTable::unlink_or_do(BoolObjectClosure* is_alive, OopClosure* f)
>>   StringTable::unlink_or_oops_do(BoolObjectClosure* is_alive,
>> OopClosure* f) ?
>
> or maybe
>
> StringTable::oops_fixup(BoolObjectClosure* is_alive, OopClosure* 
> keep_alive)

I'm going to use StringTable::unlink_or_oops_do(BoolObjectClosure* 
is_alive, OopClosure* f) for this changset.

If we don't like that name, we can change it in another changeset.

thanks,
StefanK

>
> /Per
>
>>
>> StefanK
>>
>>>
>>> That mimics the parameter naming of:
>>>  void JNIHandleBlock::weak_oops_do(BoolObjectClosure* is_alive,
>>> OopClosure* f)
>>>  void JvmtiTagMap::do_weak_oops(BoolObjectClosure* is_alive,
>>> OopClosure* f)
>>>
>>> Both of these functions applies 'f' if the is_alive answers true for
>>> the given oop and "cleans" the entry/oop if the answer is false, just
>>> like StringTable::unlink/clean does.
>>>
>>>> Or maybe unlink/oops_do has a different meaning to other people?
>>>>
>>>> A more general comment, or more like a suggestion for a future
>>>> improvement. My guess is that the overwhelming majority of the
>>>> interned String objects are old, meaning that most of the table
>>>> traversal we do every young GC is a waste of time. If the table can
>>>> provide something like young_oops_do(), which only traverses the
>>>> young references, we can probably cut another ~10% of the young GC
>>>> times in the FA CRM case.
>>>
>>> I agree.
>>>
>>> This patch is a quick fix for some low-hanging fruits.
>>>
>>> thanks for the review,
>>> StefanK
>>>>
>>>> /Per (not a reviewer)
>>>>
>>>
>>
>