RFR(xs): 8245035: Clean up os::split_reserved_memory()

[Thomas Stüfe]

Hi Coleen, Stefan,

thanks for the reviews. Please find new version here:
http://cr.openjdk.java.net/~stuefe/webrevs/8245035--clean-up-os--split_reserved_memory()/webrev.01/webrev/

Remarks inline:


On Mon, May 18, 2020 at 9:49 AM Stefan Karlsson <stefan.karlsson at oracle.com>
wrote:

> Hi Thomas,
>
> I think the patch mostly looks good. A few things that might be worth
> considering:
>
>
> https://cr.openjdk.java.net/~stuefe/webrevs/8245035--clean-up-os--split_reserved_memory()/webrev.00/webrev/src/hotspot/os/windows/os_windows.cpp.udiff.html
>
> Would you mind splitting this into three separate asserts:
>
> + assert(size > 0 && split > 0 && split < size, "Sanity");
>
> It's almost always better for those looking at failing asserts to know
> what part failed. Alternatively, add a message printing the value size
> and split.
>

Done.


>
> It's also interesting that the explicit contract for split is that it
> only needs to be > 0. I don't think this code will work if split isn't a
> non-zero multiple of os::vm_allocation_granularity()
> [SYSTEM_INFO.dwAllocationGranularity]. Maybe update the comments and
> asserts to reflect that?
>

I added asserts; note that the requirements had been checked in
os::reserve_memory too. But they are now explicit, which is clearer.


>
> Not related to your changes, but I see that we don't verify that return
> values from the calls to reserve_memory, in release builds. We should
> probably create a BUG for that.
>

I added error handling to os::split_reserved_memory. It now returns bool to
indicate success, and for now I wired that up with an assert at
ReservedSpace level.

Thank you,

Thomas


>
> Thanks,
> StefanK
>
>
> On 2020-05-14 18:12, Thomas Stüfe wrote:
> > Hi Coleen,
> >
> > thanks for the review!
> >
> > On Thu, May 14, 2020 at 5:57 PM <coleen.phillimore at oracle.com> wrote:
> >
> >>
> >>
> http://cr.openjdk.java.net/~stuefe/webrevs/8245035--clean-up-os--split_reserved_memory()/webrev.00/webrev/src/hotspot/share/memory/virtualspace.hpp.udiff.html
> >>
> >>
> >> + // If split==false, the resulting space will be just a hotspt-internal
> >> representation
> >>
> >>
> >> typo: needs an 'o'
> >>
> >>
> > Fixed.
> >
> >
> >>
> http://cr.openjdk.java.net/~stuefe/webrevs/8245035--clean-up-os--split_reserved_memory()/webrev.00/webrev/src/hotspot/share/runtime/os.cpp.udiff.html
> >>
> >> I don't think we need a pd_ version to delegate to for this.  I guess
> >> it's a convention because the other memory allocation functions do it
> >> but maybe we can slowly end this bad convention.
> >>
> > Good point. I removed the pd_ implementation and now we just have to
> direct
> > implementations of os::split_reserved_memory().
> >
> >
> >
> >> This looks like a very nice cleanup.
> >> Thank you!
> >> Coleen
> >>
> >>
> > I updated the webrev in place.
> >
> > Thanks, Thomas
> >
> >
> >> On 5/14/20 11:45 AM, Thomas Stüfe wrote:
> >>> Hi,
> >>>
> >>> may I have reviews for this small cleanup please:
> >>>
> >>> jbs: https://bugs.openjdk.java.net/browse/JDK-8245035
> >>> webrev:
> >>>
> >>
> http://cr.openjdk.java.net/~stuefe/webrevs/8245035--clean-up-os--split_reserved_memory()/webrev.00/webrev/
> >>> I need this as a preparation for
> >>> https://bugs.openjdk.java.net/browse/JDK-8243535, which I'd like to
> >> review
> >>> separately.
> >>>
> >>> os::split_reserved_memory() should be cleaned up a bit. It is used to
> >> split
> >>> reserved memory regions in order to make them independent units. This
> >> only
> >>> matters when releasing them, so after splitting these regions should be
> >>> releasable independently from each other.
> >>>
> >>> This whole thing only matters on Windows, which is the only platform
> >> with a
> >>> non-empty implementation, since virtual memory can only be released as
> a
> >>> unit (opposed to mmap api, where sub regions can be unmapped
> >> individually).
> >>> Improvements:
> >>> - the interface should be commented
> >>> - the "realloc" parameter can be removed. It has never not been true on
> >> all
> >>> code paths - had it been, it would have been an error on windows
> >> resulting
> >>> in loosing one of the two sides of the split.
> >>> - the many non-windows implementations can be combined in posix.cpp.
> >>>
> >>> Thank you,
> >>>
> >>> Thomas
> >>
>
>