RFR: 8173764: Assert in G1 BOT is wrong

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RFR: 8173764: Assert in G1 BOT is wrong

Stefan Johansson
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

Thanks,
Stefan
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8173764: Assert in G1 BOT is wrong

Thomas Schatzl
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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8173764: Assert in G1 BOT is wrong

Kim Barrett
> On Feb 21, 2017, at 6:11 AM, Thomas Schatzl <[hidden email]> 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.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: 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 <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8173764: Assert in G1 BOT is wrong

Kim Barrett
> On Feb 22, 2017, at 8:47 AM, Stefan Johansson <[hidden email]> wrote:
> […]
> Updated webrevs:
> Full: http://cr.openjdk.java.net/~sjohanss/8173764/hotspot.01/
> Inc:  http://cr.openjdk.java.net/~sjohanss/8173764/hotspot.00-01/

------------------------------------------------------------------------------
src/share/vm/gc/g1/g1BlockOffsetTable.cpp
 369 #ifndef PRODUCT
 370 void G1BlockOffsetTablePart::set_object_can_span(bool can_span) {
 371   _object_can_span = can_span;
 372 }

This should be #ifdef ASSERT, and the declaration should be
NOT_DEBUG_RETURN.  As written, an "optimize" build will fail.

------------------------------------------------------------------------------



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8173764: Assert in G1 BOT is wrong

Stefan Johansson
Thanks Kim,

On 2017-02-22 21:40, Kim Barrett wrote:

>> On Feb 22, 2017, at 8:47 AM, Stefan Johansson <[hidden email]> wrote:
>> […]
>> Updated webrevs:
>> Full: http://cr.openjdk.java.net/~sjohanss/8173764/hotspot.01/
>> Inc:  http://cr.openjdk.java.net/~sjohanss/8173764/hotspot.00-01/
> ------------------------------------------------------------------------------
> src/share/vm/gc/g1/g1BlockOffsetTable.cpp
>   369 #ifndef PRODUCT
>   370 void G1BlockOffsetTablePart::set_object_can_span(bool can_span) {
>   371   _object_can_span = can_span;
>   372 }
>
> This should be #ifdef ASSERT, and the declaration should be
> NOT_DEBUG_RETURN.  As written, an "optimize" build will fail.
>
> ------------------------------------------------------------------------------
>
Nice catch! I've heard that people want to remove optimized builds,
after this, add me to that list :)

I changed it as you suggested and now it builds when using optimized:
Full: http://cr.openjdk.java.net/~sjohanss/8173764/hotspot.02/
Inc: http://cr.openjdk.java.net/~sjohanss/8173764/hotspot.01-02/

Thanks,
Stefan
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8173764: Assert in G1 BOT is wrong

Thomas Schatzl
Hi Stefan,

On Thu, 2017-02-23 at 10:30 +0100, Stefan Johansson wrote:
> Thanks Kim,
>
> On 2017-02-22 21:40, Kim Barrett wrote:
> >
> > >
[...]

> > This should be #ifdef ASSERT, and the declaration should be
> > NOT_DEBUG_RETURN.  As written, an "optimize" build will fail.
> >
> > -----------------------------------------------------------------
> > -------------
> >
> Nice catch! I've heard that people want to remove optimized builds, 
> after this, add me to that list :)
>
> I changed it as you suggested and now it builds when using optimized:
> Full: http://cr.openjdk.java.net/~sjohanss/8173764/hotspot.02/
> Inc: http://cr.openjdk.java.net/~sjohanss/8173764/hotspot.01-02/

I would prefer if the assert text also talked
about G1BlockOffsetTablePart instead of regions, but I can see that it
might be more clear that way.

If you change this, I do not need to see a re-review - or just ignore
this comment.

Looks good. Ship it.

Thomas

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8173764: Assert in G1 BOT is wrong

Kim Barrett
In reply to this post by Stefan Johansson
> On Feb 23, 2017, at 4:30 AM, Stefan Johansson <[hidden email]> wrote:
>
> Thanks Kim,
>
> On 2017-02-22 21:40, Kim Barrett wrote:
>>> On Feb 22, 2017, at 8:47 AM, Stefan Johansson <[hidden email]> wrote:
>>> […]
>>> Updated webrevs:
>>> Full: http://cr.openjdk.java.net/~sjohanss/8173764/hotspot.01/
>>> Inc:  http://cr.openjdk.java.net/~sjohanss/8173764/hotspot.00-01/
>> ------------------------------------------------------------------------------
>> src/share/vm/gc/g1/g1BlockOffsetTable.cpp
>>  369 #ifndef PRODUCT
>>  370 void G1BlockOffsetTablePart::set_object_can_span(bool can_span) {
>>  371   _object_can_span = can_span;
>>  372 }
>>
>> This should be #ifdef ASSERT, and the declaration should be
>> NOT_DEBUG_RETURN.  As written, an "optimize" build will fail.
>>
>> ------------------------------------------------------------------------------
>>
> Nice catch! I've heard that people want to remove optimized builds, after this, add me to that list :)
>
> I changed it as you suggested and now it builds when using optimized:
> Full: http://cr.openjdk.java.net/~sjohanss/8173764/hotspot.02/
> Inc: http://cr.openjdk.java.net/~sjohanss/8173764/hotspot.01-02/
>
> Thanks,
> Stefan

looks good

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8173764: Assert in G1 BOT is wrong

Stefan Johansson
In reply to this post by Thomas Schatzl


On 2017-02-23 10:38, Thomas Schatzl wrote:

> Hi Stefan,
>
> On Thu, 2017-02-23 at 10:30 +0100, Stefan Johansson wrote:
>> Thanks Kim,
>>
>> On 2017-02-22 21:40, Kim Barrett wrote:
> [...]
>>> This should be #ifdef ASSERT, and the declaration should be
>>> NOT_DEBUG_RETURN.  As written, an "optimize" build will fail.
>>>
>>> -----------------------------------------------------------------
>>> -------------
>>>
>> Nice catch! I've heard that people want to remove optimized builds,
>> after this, add me to that list :)
>>
>> I changed it as you suggested and now it builds when using optimized:
>> Full: http://cr.openjdk.java.net/~sjohanss/8173764/hotspot.02/
>> Inc: http://cr.openjdk.java.net/~sjohanss/8173764/hotspot.01-02/
> I would prefer if the assert text also talked
> about G1BlockOffsetTablePart instead of regions, but I can see that it
> might be more clear that way.
>
> If you change this, I do not need to see a re-review - or just ignore
> this comment.
>
> Looks good. Ship it.
Thanks for the review, I'll leave it as.

Stefan
> Thomas
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8173764: Assert in G1 BOT is wrong

Stefan Johansson
In reply to this post by Kim Barrett
Thanks for the review,
Stefan

On 2017-02-23 14:46, Kim Barrett wrote:

>> On Feb 23, 2017, at 4:30 AM, Stefan Johansson <[hidden email]> wrote:
>>
>> Thanks Kim,
>>
>> On 2017-02-22 21:40, Kim Barrett wrote:
>>>> On Feb 22, 2017, at 8:47 AM, Stefan Johansson <[hidden email]> wrote:
>>>> […]
>>>> Updated webrevs:
>>>> Full: http://cr.openjdk.java.net/~sjohanss/8173764/hotspot.01/
>>>> Inc:  http://cr.openjdk.java.net/~sjohanss/8173764/hotspot.00-01/
>>> ------------------------------------------------------------------------------
>>> src/share/vm/gc/g1/g1BlockOffsetTable.cpp
>>>   369 #ifndef PRODUCT
>>>   370 void G1BlockOffsetTablePart::set_object_can_span(bool can_span) {
>>>   371   _object_can_span = can_span;
>>>   372 }
>>>
>>> This should be #ifdef ASSERT, and the declaration should be
>>> NOT_DEBUG_RETURN.  As written, an "optimize" build will fail.
>>>
>>> ------------------------------------------------------------------------------
>>>
>> Nice catch! I've heard that people want to remove optimized builds, after this, add me to that list :)
>>
>> I changed it as you suggested and now it builds when using optimized:
>> Full: http://cr.openjdk.java.net/~sjohanss/8173764/hotspot.02/
>> Inc: http://cr.openjdk.java.net/~sjohanss/8173764/hotspot.01-02/
>>
>> Thanks,
>> Stefan
> looks good
>

Loading...