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

[Stefan Karlsson]

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.
>
>> 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) ?

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