RFR(xs): 8245035: Clean up os::split_reserved_memory()
Stefan Karlsson
stefan.karlsson at oracle.com
Mon May 18 14:04:34 UTC 2020
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