RFR(xs): 8245035: Clean up os::split_reserved_memory()
Thomas Stüfe
thomas.stuefe at gmail.com
Mon May 18 14:39:59 UTC 2020
Thank you Stefan!
On Mon, May 18, 2020 at 4:04 PM Stefan Karlsson <stefan.karlsson at oracle.com>
wrote:
> Looks good.
>
> Thanks,
> StefanK
>
> On 2020-05-18 14:39, Thomas Stüfe wrote:
> > Hi Stefan,
> >
> > New version:
> >
> http://cr.openjdk.java.net/~stuefe/webrevs/8245035--clean-up-os--split_reserved_memory()/webrev.02/webrev/
> >
> > Answers inline:
> >
> > On Mon, May 18, 2020 at 2:07 PM Stefan Karlsson
> > <stefan.karlsson at oracle.com <mailto:stefan.karlsson at oracle.com>> wrote:
> >
> > Hi Thomas,
> >
> > You still have the triple-assert here:
> >
> https://cr.openjdk.java.net/~stuefe/webrevs/8245035--clean-up-os--split_reserved_memory()/webrev.01/webrev/src/hotspot/os/posix/os_posix.cpp.udiff.html
> >
> >
> > Okay, fixed.
> >
> > The split point > 0 contract is still written in os.hpp:
> >
> https://cr.openjdk.java.net/~stuefe/webrevs/8245035--clean-up-os--split_reserved_memory()/webrev.01/webrev/src/hotspot/share/runtime/os.hpp.udiff.html
> >
> >
> > I adjusted the comment as you suggested.
> >
> > If you had kept the pd_split_reserved_memory
> > os::split_reserved_memory
> > separation you could have put the asserts and contract description
> > os::split_reserved_memory, and only implemented the
> > platform-dependent
> > parts in pd_split_reserved_memory.
> >
> >
> > Removing the pd_.. version has been a request by Coleen.
> >
> >
> >
> https://cr.openjdk.java.net/~stuefe/webrevs/8245035--clean-up-os--split_reserved_memory()/webrev.01/webrev/src/hotspot/os/windows/os_windows.cpp.udiff.html
> >
> > +bool os::split_reserved_memory(char *base, size_t size, size_t
> > split) {
> > + char* const split_address = base + split;
> > + assert(size > 0 && split > 0 && split < size &&
> > + is_aligned(base, os::vm_allocation_granularity()) &&
> > + is_aligned(split_address, os::vm_allocation_granularity()),
> > + "parameter error (base=" PTR_FORMAT ", size=" SIZE_FORMAT ",
> > split="
> > SIZE_FORMAT ")",
> > + p2i(base), size, split);
> > + // We release, then re-reserve memory. This opens a very short
> > window
> > within which some else
> > + // may reserve memory in the same region, so it is not
> > guaranteed to work.
> > + bool b = release_memory(base, size) &&
> > + (attempt_reserve_memory_at(split, base) == base) &&
> > + (attempt_reserve_memory_at(size - split, split_address) ==
> > split_address);
> > + return b;
> > }
> >
> > I find this code too compact to be easily readable, but if Coleen
> > thinks
> > it look good I guess it's fine.
> >
> > I also think you missed the part where I said that I though we should
> > verify the return values in *release* builds. I don't think this adds
> > anything, since we already assert that we get the correct address in
> > debug builds. I think it would be better to just deal with that as a
> > separate issue.
> >
> >
> > I reverted the code to my original proposal, modulo the changed
> > asserts. I changed the asserts to be multi line and hopefully this is
> > less dense.
> >
> > Note the existing checks in debug builds do not cover all cases. They
> > do not cover the possibility that we won't get a mapping at all
> > because the address range had been occupied between release and the
> > two follow up reserve calls. To notice that has been the intent of my
> > error handling. As it is now, os::split_reserved_memory is inherently
> > unsafe on windows.
> >
> > But it has been that way forever, so it is not a pressing issue. We
> > can deal with this in a follow up patch.
> >
> > Thanks, Thomas
> >
> > Thanks,
> > StefanK
> >
> > On 2020-05-18 13:38, Thomas Stüfe wrote:
> > > 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 <mailto:stefan.karlsson at oracle.com>
> > <mailto:stefan.karlsson at oracle.com
> > <mailto: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 <mailto:coleen.phillimore at oracle.com>
> > > <mailto:coleen.phillimore at oracle.com
> > <mailto: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
> > > >>
> > >
> >
>
>
More information about the hotspot-runtime-dev
mailing list