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

[Stefan Karlsson]

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

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