review request (S) - 8014611: reserve_and_align() assumptions

[John Coomes]

Bengt Rutisson (bengt.rutisson at oracle.com) wrote:
> 
> Hi John,
> 
> I think your latest webrev looks good.
> 
> But I think Thomas has a point. We need to make sure that _raw_base and 
> _raw_size are always initialized to NULL/0. I still kind of think that 
> it would be safest to do this in initialization lists for all 
> constructors, but if you prefer you could maybe introduce a 
> reset_raw_base_and_size() method that you call in all places where you 
> now call save_raw_base_and_size(NULL, 0). The reset_raw_base_and_size() 
> method would then unconditionally set _raw_base = NULL and _raw_size = 0.

Hi Bengt,
 
Thanks for looking (again).  I've updated the code to always init to
null/0 (I didn't use initialization lists, though), and only set
_raw_base to non-null if os::can_release_partial_region() returns
false.  I also added some asserts to check it.  See the same url:

http://cr.openjdk.java.net/~jcoomes/8014611-reserve-and-align/

I'm going to push this.

-John

> On 5/21/13 11:51 PM, Thomas Schatzl wrote:
> > Hi,
> >
> > On Tue, 2013-05-21 at 14:27 -0700, John Coomes wrote:
> >> Bengt Rutisson (bengt.rutisson at oracle.com) wrote:
> >>> On 5/21/13 2:46 AM, John Coomes wrote:
> >>> In both ReservedSpace::save_raw_base_and_size() and
> >>> ReservedSpace::failed_to_reserve_as_requested() you have the return type
> >>> on a separate line. That does not seem to be the style in the rest of
> >>> the file.
> >> Ok; I changed the line breaks.  All the above changes are in a new
> >> webrev (at the same location):
> >>
> >> http://cr.openjdk.java.net/~jcoomes/8014611-reserve-and-align/
> >>
> >> Thanks again for the review.  I'll push this unless there are
> >> objections.
> > One minor note: I think not setting _raw_base and _raw_size inn
> > save_raw_base_and_size() in all cases can lead to junk in these
> > variables which might be confusing when debugging.
> >
> > So the check for _raw_base == NULL in ReservedSpace::release_memory() is
> > misleading/ineffective, as now, even if os::can_release_partial_memory()
> > is true, it likely contains junk that is non-zero.
> >
> > The check for os::can_release_partial_memory() in
> > ReservedSpace::release_memory() helps guaranteeing that the wrong method
> > is not called though.
> >
> > (If I follow the code correctly).
> >
> > I.e. the set_raw_base_and_size(NULL, 0) are ineffective when
> > os::can_release_partial_memory() is true.
> >
> > I'd prefer it if _raw_base and _raw_size really contained NULL/0 in case
> > of os::can_release_partial_memory() is true, and make the (single) call
> > to save_raw_baw_and_size() in ReservedSpace::reserve_and_align()
> > conditional on os::can_release_partial_memory().
> >
> > The change further does not update copyright dates :)
> >
> > Thanks,
> > Thomas
> >
> >
>