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

Mon May 18 12:07:44 UTC 2020

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

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

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.

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.

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