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

[Jiangli Zhou]

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