RFR: 8173764: Assert in G1 BOT is wrong

[Stefan Johansson]

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:
>>
>> Hi,
>>
>> On Tue, 2017-02-21 at 11:17 +0100, Stefan Johansson wrote:
>>> Hi,
>>>
>>> Please review this fix for:
>>> https://bugs.openjdk.java.net/browse/JDK-8173764
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~sjohanss/8173764/hotspot.00/
>>>
>>> Summary:
>>> 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.
>>>
>>> Testing:
>>> * 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...
Changed it.
>>   - 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".
Fixed.
>>   - 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.
Fixed.
>>
>>   - Maybe the assert could print the value it found instead of a zero.
Fixed.
>>   - copyright updates
Fixed.
>>
>> Thanks,
>>    Thomas
> +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.
Fixed.

Updated webrevs:
Full: http://cr.openjdk.java.net/~sjohanss/8173764/hotspot.01/
Inc:  http://cr.openjdk.java.net/~sjohanss/8173764/hotspot.00-01/

Thanks,
Stefan