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

[Ioi Lam]

On 8/28/18 3:18 PM, Jiangli Zhou wrote:
> On 8/28/18 2:30 PM, Ioi Lam wrote:
>
>> 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();
>>     }
>>   }
> For any non-zero value, it is aligned up to the heap alignment and the 
> VM tries to reserve at the requested address when possible. For 0 
> HeapBaseMinAddress, it just maps at any arbitrary address. 

The handling of zero is OS-specific. In any case, if the OS picks any 
arbitrary address, it will be over-ruled by the following "if", so 
specifying 0 would be as-if you didn't specify it.

     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();
     }

This is pretty messy. I think adding a comment will not add much value 
unless I start writing an essay here.

Thanks
- Ioi
> zero is a special but acceptable case for HeapBaseMinAddress in 
> ReservedHeapSpace::initialize_compressed_heap(). I was hoping a 
> comment could be added to clarify that. But, I'm okay if you decide to 
> not add comment with the change.
>
> Thanks,
> Jiangli
>>
>> 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,
>>>>>>
>>>
>>
>