Request for review (S): 8001049: VM crashes when running with large -Xms and not specifying ObjectAlignmentInBytes

[Bengt Rutisson]

Hi Harold,

Thanks for the review and thanks for spotting the spelling mistake! 
Fixed it.

Bengt

On 3/11/13 4:33 PM, harold seigel wrote:
> Hi Bengt,
>
> These changes also look good.
>
> One very minor nit is that InitialHeapSize (IniitalHeapSize)is 
> misspelled in one of the comments.
>
> Thanks, Harold
>
> On 3/11/2013 10:42 AM, Bengt Rutisson wrote:
>>
>> Hi Harold,
>>
>> Thanks for looking at these changes.
>>
>> On 3/8/13 6:41 PM, harold seigel wrote:
>>> The change looks good.
>>
>> Thanks, I had to update the webrev. Let me know what you think about 
>> that solution.
>>
>>> Perhaps this problem wasn't seen before because this scenario hadn't 
>>> been tested?
>>
>> Yes, this is not tested and it is actually difficult to add a 
>> meaningful test for it. The problem will only appear on machines with 
>> more than 32 GB memory. We don't have many of them in our testing. 
>> There is no way to tell the test infrastructure that the test has to 
>> be run on a large machine. So, if I write the test (which would be 
>> very simple) it would most of the time just consume time and 
>> resources without testing anything since it would be run on too small 
>> machines. And I would have to write the test to "pass" on those.
>>
>> Bengt
>>
>>>
>>> Harold
>>>
>>> On 3/8/2013 11:17 AM, Vladimir Kozlov wrote:
>>>> The change is reasonable.
>>>>
>>>> So why we did not see this problem before?
>>>>
>>>> Thanks,
>>>> Vladimir
>>>>
>>>> On 3/8/13 5:20 AM, Bengt Rutisson wrote:
>>>>>
>>>>> Hi all,
>>>>>
>>>>> Could I have a couple of reviews for this change please?
>>>>> http://cr.openjdk.java.net/~brutisso/8001049/webrev.00/
>>>>>
>>>>> I'm sending this to both the GC and the runtime teams since I think
>>>>> compressed oops is mixed responsibility for both teams.
>>>>>
>>>>> Background (mostly from the bug report):
>>>>>
>>>>> Hotspot crashes if it is run with a large initial size:
>>>>>
>>>>>  >./java -Xms70g -version
>>>>> #
>>>>> # A fatal error has been detected by the Java Runtime Environment:
>>>>> #
>>>>> # SIGSEGV (0xb) at pc=0x0000000000000000, pid=14132, 
>>>>> tid=140305232803584
>>>>>
>>>>> The reason is that we enable UseCompressedOops but we use the default
>>>>> value for ObjectAlignmentInBytes. With a large heap size we would 
>>>>> have
>>>>> needed to adjust the object alignment to be able to use compressed 
>>>>> oops.
>>>>>
>>>>> However, after reviewing the code it looks like the fix is not to 
>>>>> try to
>>>>> adjust the object alignment but rather to not enable compressed 
>>>>> oops for
>>>>> large heaps. If someone wants to use compressed oops on a very large
>>>>> heap they need to explicitly set both UseCompressedOops and
>>>>> ObjectAlignmentInBytes on the command line. As far as I can tell 
>>>>> this is
>>>>> how it is intended to work.
>>>>>
>>>>> Here is the reason for the crash and the rational behind the fix:
>>>>>
>>>>> In Arguments::set_ergonomics_flags() we check that the max heap 
>>>>> size is
>>>>> small enough before we enable compressed oops:
>>>>>
>>>>>    if (MaxHeapSize <= max_heap_for_compressed_oops()) {
>>>>> #if !defined(COMPILER1) || defined(TIERED)
>>>>>      if (FLAG_IS_DEFAULT(UseCompressedOops)) {
>>>>>        FLAG_SET_ERGO(bool, UseCompressedOops, true);
>>>>>      }
>>>>> #endif
>>>>>
>>>>> And after that we print a warning message if the heap is too large:
>>>>>
>>>>>      if (UseCompressedOops && !FLAG_IS_DEFAULT(UseCompressedOops)) {
>>>>>        warning("Max heap size too large for Compressed Oops");
>>>>>        FLAG_SET_DEFAULT(UseCompressedOops, false);
>>>>>        FLAG_SET_DEFAULT(UseCompressedKlassPointers, false);
>>>>>      }
>>>>>
>>>>> Now the problem is that when we don't set the max heap size on the
>>>>> command line it will be adjusted based on the initial size (-Xms) and
>>>>> this happens in Arguments::set_heap_size(), which is called *after*
>>>>> Arguments::set_ergonomics_flags() has been called. So, when we do the
>>>>> check against the max size in Arguments::set_ergonomics_flags(), we
>>>>> check against the default value for the max size. This value fits 
>>>>> well
>>>>> with a compressed heap, so we enable compressed oops and crash 
>>>>> later on
>>>>> when we can't address the upper part of the heap.
>>>>>
>>>>> The fix is to move the call to set_heap_size() to *before* the 
>>>>> call to
>>>>> set_ergonomics_flags(). This way the check is done against the 
>>>>> correct
>>>>> value. This has two effects:
>>>>>
>>>>> 1) We don't enable UseCompressedOops on heap sizes that are too large
>>>>> 2) If someone sets -XX:+UseCompressedOops on the command line but
>>>>> specifies a too large heap a warning message will be logged and
>>>>> UseCompressedOops will be turned off.
>>>>>
>>>>> I am always hesitant to rearrange the order of calls in
>>>>> Arguments::parse(). But in this case it is necessary to get the 
>>>>> correct
>>>>> behavior and I think it is safe. As far as I can tell there is no 
>>>>> other
>>>>> code between the two methods that try to read the MaxHeapSize value.
>>>>>
>>>>> Thanks,
>>>>> Bengt
>>>
>>
>