RFR(xs): 8245035: Clean up os::split_reserved_memory()
Thomas Stüfe
thomas.stuefe at gmail.com
Mon May 18 12:39:02 UTC 2020
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>
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>> 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>> 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