Review request: 8015422: Large performance hit when the StringTable is walked twice in Parallel Scavenge
per.liden at oracle.com
Tue May 28 08:41:10 UTC 2013
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:
>>> On 2013-05-27 13:50, Stefan Karlsson wrote:
>>>> 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:
>>>> 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,
>>> 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) ?
> 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) ?
StringTable::oops_fixup(BoolObjectClosure* is_alive, OopClosure* keep_alive)
>> 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,
>>> /Per (not a reviewer)
More information about the hotspot-gc-dev