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

[John Coomes]

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.

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

-John