RFR(s): 8144949: TestOptionsWithRanges -XX:NUMAInterleaveGranularity=2147483648 crashes VM

[Jon Masamitsu]

On 12/16/2015 04:50 AM, Jesper Wilhelmsson wrote:
> Den 16/12/15 kl. 02:42, skrev sangheon:
>> Hi Jon again,
>>
>> On 12/15/2015 04:50 PM, sangheon wrote:
>>> Hi Jon,
>>>
>>> Thank you for reviewing this.
>>>
>>> On 12/15/2015 04:38 PM, Jon Masamitsu wrote:
>>>>
>>>>
>>>> On 12/15/2015 3:47 PM, sangheon wrote:
>>>>> Hi Jesper,
>>>>>
>>>>> Thanks for the review!
>>>>>
>>>>> Sangheon
>>>>>
>>>>>
>>>>> On 12/15/2015 03:33 PM, Jesper Wilhelmsson wrote:
>>>>>> Looks good!
>>>>>> /Jesper
>>>>>>
>>>>>> Den 16/12/15 kl. 00:10, skrev sangheon:
>>>>>>> Hi all,
>>>>>>>
>>>>>>> Could I get some reviews for this change?
>>>>>>>
>>>>>>> Current 32bit binary with NUMAInterleavingGranularity=2g on 
>>>>>>> server mode
>>>>>>> fires an
>>>>>>> assert on Windows as we are proceeding without memory allocation 
>>>>>>> failure
>>>>>>> check
>>>>>>> at CodeCache::reserve_heap_memory.
>>>>>>>
>>>>>>> I think the constraint function can be removed with maximum 
>>>>>>> range of
>>>>>>> 2G/8192G.
>>>>>>> These are the maximum available memory on Windows and smaller 
>>>>>>> values can
>>>>>>> be used
>>>>>>> but I wanted to avoid adding artificial limit. With this 
>>>>>>> limitation, current
>>>>>>> constraint function for overflow check is not needed.
>>>>>>> And we need to check allocation failure.
>>>>>>>
>>>>>>> This issue is not reproducible with client mode as the different 
>>>>>>> default
>>>>>>> value
>>>>>>> of ReservedCodeCacheSize sets SegmentedCodeCache to false. And
>>>>>>> CodeCache::reserve_heap_memory() is called only when 
>>>>>>> SegmentedCodeCache is
>>>>>>> enabled.
>>>>>>>
>>>>>>> Skipped adding a test as TestOptionsWithRanges.java is enough 
>>>>>>> when this is
>>>>>>> combined with nightly vm option rotation.
>>>>>>>
>>>>>>> CR: https://bugs.openjdk.java.net/browse/JDK-8144949
>>>>>>> Webrev: http://cr.openjdk.java.net/~sangheki/8144949/webrev.00
>>>>>>> Testing: JPRT (with TestOptionsWithRanges.java enabled), manual 
>>>>>>> tests on
>>>>>>> Windows
>>>>>>> machine(to test several option combination).
>>>>
>>>> Sangheon,
>>>>
>>>> Could you check if the changes in globals.hpp can be put in
>>>>
>>>> "os/windows/vm/globals_windows.hpp"
>>>>
>>>> Since this is a windows specific flag, it should work to
>>>> go in windows.hpp.
>>> This flag is only used for Windows (as its description says), so I 
>>> am not sure
>>> whether we need to move it to windows.hpp.
>> I understood your comment to move my changes(range and constraint) to
>> globals_windows.hpp. i.e. making the flag as product_pd. Since this 
>> flag is only
>> used on Windows, I am not sure it is a good idea.
>> If you are suggesting to move the whole definition of the flag into
>> globals_windows.hpp, let me consider it. I'm not sure whether it's 
>> feasible or not.
>
> The description of the flag kind of mentions that this is a Windows 
> specific flag, but it's not fool prof. It's possible that this flag is 
> used on non-Windows set ups as well. For instance applications could 
> use the same set of flags on multiple platforms. Removing this flag on 
> non-Windows platforms would be a breaking change in this case and will 
> most likely require a CCC.
> /Jesper

Jesper, you're right about the side effects of moving the code to
the globals_windows.hpp.

Sangheon,  forget about that suggestion.

Changes look good.

Jon

>
>>
>> Thanks,
>> Sangheon
>>
>>
>>>
>>>>
>>>> On 32bit upper limit is 2G.  Is that the largest value that can be 
>>>> used
>>>> without having to check for arithmetic overflow?
>>> No.
>>> That is Windows OS limitation that each process would use. And 
>>> within this
>>> range we don't need to check arithmetic overflow as it will never 
>>> happen.
>>>
>>> Thanks,
>>> Sangheon
>>>
>>>
>>>>
>>>> Jon
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Sangheon
>>>>>>>
>>>>>
>>>>
>>>
>>