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

[Ioi Lam]

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