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

[Ron Durbin]

John

What tests have you Run already or plan to before checkin?
The change looks good but can you answer the above question?
Thx for fixing this.

This will be my only response to this review as I am leaving on vacation.

Ron

> -----Original Message-----
> From: Bengt Rutisson
> Sent: Monday, May 20, 2013 5:33 AM
> To: hotspot-runtime-dev at openjdk.java.net
> Subject: Re: review request (S) - 8014611: reserve_and_align() assumptions
> 
> 
> 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.htm
> l
> 
> 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
> >
>