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

[Per Lidén]

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)

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