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

[John Coomes]

Thomas Schatzl (thomas.schatzl at oracle.com) 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().

Hi Thomas,

Thanks for taking a look.

The invariant is/was _raw_base and _raw_size are only used/touched
when os::can_release_partial_memory() returns false.  So they could
contain junk, but the junk was never used.

Nevertheless, I've updated the code to always init them to NULL/0, and
only store non-NULL/0 values when os::can_release_partial_region()
returns false.  See the same url:

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

I'm going to push this.

> The change further does not update copyright dates :)

Yea, I'm going to skip those.

-John