RFR: 8173764: Assert in G1 BOT is wrong
stefan.johansson at oracle.com
Wed Feb 22 13:47:43 UTC 2017
Thanks for the reviews Thomas and Kim,
On 2017-02-21 17:00, Kim Barrett wrote:
>> On Feb 21, 2017, at 6:11 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
>> On Tue, 2017-02-21 at 11:17 +0100, Stefan Johansson wrote:
>>> Please review this fix for:
>>> The changed assert, asserted that there was an object at the start of
>>> the heap. What we want to assert is that no object span over region
>>> boundaries. That basically means that there is an object at the start
>>> of every region, with one exception, continues humongous regions.
>>> They need special handling since they don't contain an object at the
>>> start of the region.
>>> * JPRT
>>> * RBT tier2 + tier3
>> Some comments:
>> - maybe the CR title could be a bit more specific, like "G1 BOT
>> wrongly assumes that objects must always begin at the start of
>> G1BlockOffsetTablePart". Sure, that is quite long...
>> - could the change make the _object_can_span variable assert-only? I
>> prefer if that variable were only set when it's used. It also makes it
>> clear that it is some variable used for assertions only.
Good point, fixed.
>> - the variable should have a comment what it is supposed to indicate,
>> not only a description of one of it's values. Maybe something like
>> "Indicates that an object can span/reach into
>> this G1BlockOffsetTablePart".
>> - the setter should have the same name as the variable to set. The
>> current name kind of introduces a higher level concept ("humongous")
>> into this code unnecessarily.
>> - Maybe the assert could print the value it found instead of a zero.
>> - copyright updates
> +1 on Thomas's comments. For the assert, make it say something like
> "Object crossed region boundary", and then the kind of additional
> information needed should become more obvious.
More information about the hotspot-gc-dev