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

[Ioi Lam]

Hi Jiangli,


I think the current code is pretty self-explanatory -- we will try to 
map at any arbitrary address specified by the user, but there's no 
guarantee that it will succeed.

I don't think there's need to specifically talk about 
HeapBaseMinAddress==0, as the user might just as well specify 
HeapBaseMinAddress=12345678 and that doesn't affect the logic.

   // Attempt to alloc at user-given address.
   if (!FLAG_IS_DEFAULT(HeapBaseMinAddress)) {
     try_reserve_heap(size + noaccess_prefix, alignment, large, 
aligned_heap_base_min_address);
     if (_base != aligned_heap_base_min_address) { // Enforce this exact 
address.
       release();
     }
   }

Thanks
- Ioi

On 8/28/18 1:45 PM, Jiangli Zhou wrote:
> 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,
>>>>
>