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

[Per Liden]

On 2013-05-29 13:10, Stefan Karlsson wrote:
> 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.

Sounds good.

/Per