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

[sangheon]

Hi Jon and Jesper,

On 12/16/2015 10:00 AM, Jon Masamitsu wrote:
>
>
> 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.
Thank you Jesper, I didn't know about this.

>
> Sangheon,  forget about that suggestion.
Okay, Jon.
I will proceed as is.

Thanks,
Sangheon


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