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

[Bengt Rutisson]

Hi John,

Thanks for the extra explanation. It really helped!

Some comments inline...

On 5/18/13 1:30 AM, John Coomes wrote:
> Bengt Rutisson (bengt.rutisson at oracle.com) wrote:
>> Hi John,
>>
>> On 5/15/13 6:59 PM, John Coomes wrote:
>>
>>      Please review this change for hs24:
>>
>>              8014611: reserve_and_align() assumptions are invalid on windows
>>
>>              http://cr.openjdk.java.net/~jcoomes/8014611-reserve-and-align/
>>
>>      The webrev has more info about the problem and the fix.
>>
>>      -John
>>
>>
>> I don't really understand how this fix helps the Windows issue. The problem on
>> Windows is that if you try to release any part of a reserved region it will
>> actually release the whole region instead of the part that you specified. Or
>> did I miss understand the problem?
> First, thanks for taking a look.  And you understand the problem
> correctly.
>
>> If that is the underlying issue then I would have expected this code in
>> ReservedSpace::align_reserved_region():
>>
>>    83   if (beg_delta != 0) {
>>    84     os::release_partial_region(addr, beg_delta);
>>    85   }
>>    86
>>    87   if (end_delta != 0) {
>>    88     char* release_addr = (char*) (s + beg_delta + required_size);
>>    89     os::release_partial_region(release_addr, end_delta);
>>    90   }
>>
>> to be:
>>
>> if (beg_delta != 0 && os::can_release_partial_region()) {
>>    ...
>> }
>>
>> if (end_delta != 0 && os::can_release_partial_region()) {
>>    ...
>> }
>>
>> And that os::release_partial_region() on Windows should be implemented as:
>>
>> bool os::release_partial_region(char* addr, size_t bytes) {
>>    ShouldNotReachHere();
>> }
>>
>> I guess I am missing something. Can you explain in more detail why your
>> approach solves the Windows uncommit problem?
> The proposed fix allows os::release_partial_region() to be called on
> windows (even though can_release_partial_region() returns false
> there).  That's because on windows, release_partial_region() calls
> uncommit_memory() to return any pages that may have been committed to
> back the partial region, which is allowed by the win api.  As you say,
> it can't return part of the virtual address range on windows, as can
> be done on posix with mmap, but it can uncommit pages.

Right. I missed that os::release_partial_region() on Windows only did 
uncommit.

>
> The currently implemented semantics of release_partial_region() are:
>
> 	If the o/s supports releasing part of a virtual address
> 	reservation, do so using release_memory().  Otherwise (on
> 	systems where the VA reservation must be released in its
> 	entirety), uncommit any pages backing the partial region.
>
> I started out with a solution like you sketched above (with
> release_partial_region() on windows essentially a no-op), but thought
> that it would be more frugal to uncommit any memory that might back
> the virtual addresses.  The current code will not benefit from this,
> but if another use of release_partial_region() is added, that code
> might.
>
> Some options:
>
> 	I can add the description above as a comment if that would
> 	help, and/or
>
> 	rename release_partial_region() to something that better
> 	indicates the semantics (suggestions welcome), or
>
> 	make release_partial_region() a no-op on windows, with a small
> 	loss in future-proofing

I would like to suggest one more option:

* Remove the os::release_partial_region() method
* Always store raw_base and raw_size
* Make ReservedSpace::release_memory() only use 
os::can_release_partial_region() to decide whether to use the raw_base 
or the "normal" base address for releasing
* Let ReservedSpace::align_reserved_region() be implemented as:


if (beg_delta != 0)
   if (os::can_release_partial_region()) {
     os::release_memory(addr, beg_delta);
   } else {
     os::uncommit_memory(addr, beg_delta);
   }
}

if (end_delta != 0) {
   if (os::can_release_partial_region()) {
     char* release_addr = (char*) (s + beg_delta + required_size);
     os::release_memory(release_addr, end_delta);
   } else {
     os::uncommit_memory(release_addr, end_delta);
   }
}


For me this is more intuitive than calling a method called 
os::release_partial_region() on platforms that answer false on 
os::can_release_partial_region().

Thanks,
Bengt

>
> -John