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

[John Coomes]

Bengt Rutisson (bengt.rutisson at oracle.com) wrote:
> On 5/21/13 2:46 AM, John Coomes wrote:
> > ...
> > So I tried my second alternative (renaming release_partial_region()),
> > and was able to move the method from platform-dependent code into
> > os.cpp.  It's simpler than always using _raw_base, let me know what
> > you think.  Here's a webrev of just the rename (i.e., relative to my
> > original change):
> >
> > 	http://cr.openjdk.java.net/~jcoomes/8014611-rename/
> >
> > Here's a combined webrev (relative to unmodified hs24):
> >
> > 	http://cr.openjdk.java.net/~jcoomes/8014611-combo/
> 
> Good idea! I like this much better!

Yes, me too :-).

> Some minor comments based on the combined webrev:
> 
> In theory you don't need to save the raw_size since the size does not 
> matter when you release memory on Windows. But it looks more consistent 
> to do save the size too, so I think it is probably better to keep it.
> 
> In ReservedSpace::release_memory() I find this a bit hard to read:
> 
>    const bool ok = _raw_base == NULL ?
>      os::release_memory(default_addr, default_size) :
>      os::release_memory(_raw_base, _raw_size);
> 
> For me I think an if statement here would be faster to read.

Sure, I changed it to an if.

> The constructors for ReservedSpace are a mess.  ...

They got a little better in hs25 :-).

> ...                                            But it seems like all 4 
> of them call ReservedSpace::initialize(), so it should be enough to have 
> the call to save_raw_base_and_size(NULL, 0) in 
> ReservedSpace::initialize() rather than in the constructors.
> 
> I realize that this is not 100% safe since a constructor could 
> potentially call release_memory() before it calls initialize(). So, a 
> safer alternative would be to initialize _raw_base and _raw_size in the 
> constructors and remove the call to save_raw_base_and_size(NULL, 0) from 
> initialize(). If you would like to go with that approach I would prefer 
> to reset these values in an initialization list rather than with a call 
> to save_raw_base_and_size(NULL, 0).

Use of _raw_base/_raw_size are somewhat localized now; I'd rather not
spread them around more.

> The comment for ReservedSpace::release_memory() is:
> 
> // On some systems, the address returned by os::reserve_memory() is the only
> // addr that can be passed to os::release_memory().  If alignment was 
> done by
> // this class, that original address is _raw_base.
> 
> Would it be worth explicitly mentioning Windows here?

Sure, done.

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

-John