RFR (S): 8144714 Add extension point to G1 evacuation closures

[Mikael Gerdin]

Hi Kim,

On 2015-12-10 04:50, Kim Barrett wrote:
> On Dec 4, 2015, at 10:18 AM, Mikael Gerdin <mikael.gerdin at oracle.com> wrote:
>>
>> Hi all,
>>
>> Please review this small change to add a hook in the G1 evacuation path.
>> This allows extension code to register regions which are otherwise interesting to the GC and get notifications when pointers into such regions are encountered.
>>
>> Testing: JPRT, gc test suite
>> Performance:
>> This additional branch in G1ParScanClosure introduces a slight regression of about 1-2% in GC pause times. I have some performance fixes to make up for that regression in the pipe.
>>
>> webrev: http://cr.openjdk.java.net/~mgerdin/8144714/webrev.0/
>> bug: https://bugs.openjdk.java.net/browse/JDK-8144714
>>
>> Thanks
>> /Mikael
>
> Another possible optimization to mitigate the extension point?
>
> I think before the addition of the extension point support,
> is_in_cset_or_humongous should have been _value != NotInCSet,
> and is_default should have been _value == NotInCSet (the latter is
> part of the proposed change).
>
> With the extension point support, it might be better to use the
> encoding
>
>    Humongous = -2
>    Ext = -1
>
> With that encoding, is_in_cset_or_humongous can be written as
>
>    static_cast<unsigned>(_value + 1) > 1 // NotInCSet + 1
>
> With that encoding, is_valid would need to revert back to comparing
> against Humongous rather than Ext as the boundary value.
>
> This encoding would change
>
>    branch if _value > 0 to handle_in_cset_or_humongous
>    branch if _value == Humongous to handle_in_cset_or_humongous
>    // ! is_in_cset_or_humongous
>
> to
>
>    branch if (unsigned)(_value + 1) > 1 to handle_in_cset_or_humongous
>    // ! is_in_cset_or_humongous
>
> If the ! is_in_cset_or_humongous case is common, this could be a win.
>
> I also looked at this encoding:
>
>    Ext = -2
>    NotInCSet = -1
>    Humongous = 0
>    Young = 1
>    Old = 2
>
> is_in_cset := _value > Humongous
> is_in_cset_or_humongous := _value >= Humongous
>
> But there seem to be a bunch of places that break the abstraction and
> have assumptions about the non-negative part of the current encoding.

Thanks for the suggestions!
Since I've already stated my plan to do further performance work in 
order to reduce the impact of this change and you've stated that you 
don't plan to fully review this change would you be ok with me pushing 
this as-is and look into investigating your suggestions post-integration?

/Mikael-


>