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

[Bengt Rutisson]

On 5/21/13 2:46 AM, John Coomes wrote:
> Bengt Rutisson (bengt.rutisson at oracle.com) 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().
> Agreed; I myself noticed that calling release_partial_region() when
> can_release_partial_region() returns false was a bit
> counter-intuitive.
>
> I looked at always saving _raw_base and _raw_size and using them
> unconditionally when releasing memory.  I'd have to save them after
> every call to reserve_memory() or release_memory(), which is about a
> dozen places (including in the code snippets above, where I have to
> save the raw values only if can_release_partial_region() returns
> true).  So the first snippet above would have to be:
>
> if (beg_delta != 0)
>     if (os::can_release_partial_region()) {
>       os::release_memory(addr, beg_delta);
>       save_raw_base_and_size(addr + beg_delta, size - beg_delta);
>     } else {
>       os::uncommit_memory(addr, beg_delta);
>     }
> }
>
> Similarly for the second snippet.  And when releasing memory, I'd
> still want to check that _raw_base != NULL before using it.  It
> started to get a bit ugly saving the values everywhere.

Good point. I totally agree.

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

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.


The constructors for ReservedSpace are a mess. 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).


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?


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.

Thanks,
Bengt

>
> -John