RFR[XS] 8210043 Invalid assert(HeapBaseMinAddress > 0) in ReservedHeapSpace::initialize_compressed_heap

[Jiangli Zhou]

Hi Ioi,

Thanks for the additional explanation. Just did some more experiments. 
With '-Xmx1024m -XX:HeapBaseMinAddress=0', the VM allocates the java 
heap at the same address range as with '-Xmx1024m' (when the default 
HeapBaseMinAddress is taking effect). That eased my concern. Also, the 
existing argument checks guarantee that negative value cannot be passed 
to HeapBaseMinAddress. Based on those, it seems removing the assert is 
ok. Could you please add a comment in 
ReservedHeapSpace::initialize_compressed_heap() with the explanation for 
the 0 HeapBaseMinAddress case?

Thanks,

Jiangli


On 8/28/18 11:18 AM, Ioi Lam wrote:
> Hi Jiangli,
>
> Thanks for the review.
>
> The VM doesn’t really have any ideas of what’s a good value for HeapBaseMinAddress. All it does is to try to map the address as given by the user. If it fails, then the VM will ask the os to allocate a block at an address picked by the os.
>
> The mapping at 0 will invariably fail, because on all OSes this range is not mappable by the user program.
>
> When MaxHeapSize is not set, the GC ergonomics code tries to pick a “good” address that is likely to work well for compressed oops, etc. I think that’s why it overrides HeapBaseMinAddress (although there’s no comment so I don’t know exactly why). However, if MaxHeapSize and HeapBaseMinAddress are both specified by the user, the user probably knows what they are doing and I don’t think the VM should interfere.
>
> As I mentioned above, picking any HeapMinAddress is safe as the VM will simply ignore the values that don’t work.
>
> Thanks
> Ioi
>
>
>> On Aug 28, 2018, at 10:46 AM, Jiangli Zhou <jiangli.zhou at oracle.com> wrote:
>>
>> Hi Ioi,
>>
>> This probably is not a safe change and masks the bug that happens elsewhere. When running with '-XX:HeapBaseMinAddress=0 -version', Arguments::set_heap_size() resets HeapBaseMinAddress to DefaultHeapBaseMinAddress. That's why the assert in ReservedHeapSpace::initialize_compressed_heap() is not hit.
>>
>> I just ran '-Xmx1024m -XX:HeapBaseMinAddress=0' in gdb. Adjusting the HeapBaseMinAddress is not happening in this case because MaxHeapSize is not default. That causes 0 being seen as the HeapBaseMinAddress later in ReservedHeapSpace::initialize_compressed_heap().
>>
>> Thanks,
>>
>> Jiangli
>>
>>
>>> On 8/27/18 11:18 PM, Ioi Lam wrote:
>>> Hi, please review this one-liner change:
>>>
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8210043
>>>
>>> There is code that ensures HeapBaseMinAddress >= DefaultHeapBaseMinAddress,
>>> but that happens only when MaxHeapSize is default, so the assert doesn't match
>>> the existing behavior.
>>>
>>> The JVM works just fine if we remove the assert.
>>>
>>> $ hg diff src/hotspot/share/memory/virtualspace.cpp
>>> diff -r 2e928420389d src/hotspot/share/memory/virtualspace.cpp
>>> --- a/src/hotspot/share/memory/virtualspace.cpp    Tue Aug 21 20:23:34 2018 -0700
>>> +++ b/src/hotspot/share/memory/virtualspace.cpp    Mon Aug 27 23:05:18 2018 -0700
>>> @@ -490,7 +490,6 @@
>>>     guarantee(size + noaccess_prefix_size(alignment) <= OopEncodingHeapMax,
>>>               "can not allocate compressed oop heap for this size");
>>>     guarantee(alignment == MAX2(alignment, (size_t)os::vm_page_size()), "alignment too small");
>>> -  assert(HeapBaseMinAddress > 0, "sanity");
>>>
>>>     const size_t granularity = os::vm_allocation_granularity();
>>>     assert((size & (granularity - 1)) == 0,
>>>