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

[John Coomes]

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.

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/

-John