RFR: 8173764: Assert in G1 BOT is wrong

[Kim Barrett]

> 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...
> 
>  - 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.
> 
>  - 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
> 
> 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.