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

[Stefan Karlsson]

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.

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?

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.

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