RFR: 8166663: Simplify oops_on_card_seq_iterate_careful interface

[Mikael Gerdin]

Hi,

On 2016-09-26 10:06, Thomas Schatzl wrote:
> Hi Kim,
>
> On Sun, 2016-09-25 at 19:57 -0400, Kim Barrett wrote:
>> Please review this simplification of the interface and implementation
>> of HeapRegion::oops_on_card_seq_iterate_careful. It only has one
>> caller, G1RemSet::refine_card, which does not use some of the
>> presently supported generality.
>>
>> By making these changes we simplify the analysis and code changes
>> required to address the main task of JDK-8160369.
>>
>> Specific issues include:
>>
>> (1) This function has a bool filter_young parameter, which is used to
>> control and conditionalize its behavior. However, the only caller
>> always provides a true value for that parameter.
>>
>> (2) This function returns either NULL (to indicate success) or the
>> start address of a possibly incompletely initialized object that
>> caused heap parsing to fail. The caller uses this value to decide
>> whether the operation was successful (NULL), but doesn't use the
>> actual failure value (the address where parsing failed). Instead,
>> there is a comment suggesting a more complex to implement response to
>> failure might be used in the future.
>>
>> Unlike CMS, with G1 the situation where parsing may fail is quite
>> rare. This will be discussed further with the fix for JDK-8166607. So
>> extra work here isn't worthwhile, and the present response of just
>> requeuing the card for later reconsideration is adequate.
>>
>> Since the caller presently only uses the result as a (inverted)
>> boolean success/failure, changing the result to a (non-inverted)
>> success/failure indication further simplifies both the caller and the
>> callee.
>>
>> CR:
>> https://bugs.openjdk.java.net/browse/JDK-8166663
>>
>> Webrev:
>> http://cr.openjdk.java.net/~kbarrett/8166663/webrev.00/
>>
>> Testing:
>> JPRT, local jtreg hotspot_all
>>
>
>   - please add braces around the statement in the statement executed
> when the boolean expression is true in line 369 heapRegion.cpp.
>
>   - maybe add a sentence like "Iterate over the the card in the card
> table specified by card_ptr/mr, applying cl on all references." at the
> description in line 656 in heapRegion.hpp. Or just try to make the
> description a bit more readable.
>
> In any case I do not need re-review for either trivial change.

I'm fine with this change including Thomas' suggestions above.

/Mikael

>
> Thanks,
>   Thomas
>