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

[John Coomes]

Ron Durbin (ron.durbin at oracle.com) wrote:
> 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.

Hi Ron,

Thanks for the review.  In addition to the tests mentioned in [1], I
also did some ad-hoc testing on windows, including GCOld and a simple
program I wrote that induces lots of GCs, with and without
compressed-oops.  I wasn't able to run all the test suites on windows;
however, the code is exercised almost exclusively at startup so I'm
comfortable with it.

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

Enjoy :-).

-John

[1] http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2013-May/007301.html


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