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

[Bengt Rutisson]

Hi again John,

One more detail...

If you want to have a test that verifies your change you could try to 
adapt the test that I wrote for hs25 before I realized that this code is 
not used in hs25. The test, VirtualSpace::test_reserve_aligned(), is 
available at the end of this page from my review request:

http://cr.openjdk.java.net/~brutisso/8012915/webrev.00/src/share/vm/runtime/virtualspace.cpp.udiff.html

Bengt

On 5/20/13 1:27 PM, Bengt Rutisson wrote:
>
> 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
>