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

[Bengt Rutisson]

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.

Thanks,
Bengt

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