RFR: 8166663: Simplify oops_on_card_seq_iterate_careful interface
mikael.gerdin at oracle.com
Mon Sep 26 12:09:07 UTC 2016
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
>> 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.
More information about the hotspot-gc-dev