[9] RFR(S): 8172731: runtime/Thread/TooSmallStackSize.java fails on solaris-x64 with product build

Tue Jan 17 07:50:05 UTC 2017

Thanks, Götz!

Best regards,
Tobias

On 17.01.2017 07:17, Lindenmaier, Goetz wrote:
> That's good, thanks a lot!
> 
> Best regards,
>   Goetz
> 
>> -----Original Message-----
>> From: Tobias Hartmann [mailto:tobias.hartmann at oracle.com]
>> Sent: Montag, 16. Januar 2017 16:26
>> To: David Holmes <david.holmes at oracle.com>; Lindenmaier, Goetz
>> <goetz.lindenmaier at sap.com>; daniel.daugherty at oracle.com; hotspot-
>> dev at openjdk.java.net Developers <hotspot-dev at openjdk.java.net>
>> Subject: Re: [9] RFR(S): 8172731: runtime/Thread/TooSmallStackSize.java fails
>> on solaris-x64 with product build
>>
>> Hi,
>>
>> Goetz suggested (off-thread) to add a comment to the patch stating the
>> reason for the large stack size:
>> http://cr.openjdk.java.net/~thartmann/8172731/webrev.01/
>>
>> Are you fine with that?
>>
>> Thanks,
>> Tobias
>>
>> On 16.01.2017 13:38, Tobias Hartmann wrote:
>>> Hi David,
>>>
>>> On 16.01.2017 13:33, David Holmes wrote:
>>>> Sorry I missed answering the "should we file a studio compiler bug"
>> question. Don't we generate the MachNodeGenerator code using adlc? What
>> does the thing we generate actually look like?
>>>
>>> Yes, the C(++) code is generated using adlc. It looks like this:
>>>
>>> //------------------------- MachNode Generator ---------------
>>> // A switch statement on the dense-packed user-defined type system
>>> // that invokes 'new' on the corresponding class constructor.
>>>
>>> MachNode *State::MachNodeGenerator(int opcode){
>>>   switch(opcode) {
>>>   case loadB_rule: {
>>>       loadBNode *node = new loadBNode();
>>>       return node;
>>>     }
>>>   case loadB2L_rule: {
>>>       loadB2LNode *node = new loadB2LNode();
>>>       return node;
>>>     }
>>>   case loadUB_rule: {
>>>       loadUBNode *node = new loadUBNode();
>>>       return node;
>>>     }
>>>   case loadUB2L_rule: {
>>>
>>> [...]
>>>
>>>>
>>>> David
>>>>
>>>> On 16/01/2017 10:21 PM, David Holmes wrote:
>>>>> On 16/01/2017 10:15 PM, Tobias Hartmann wrote:
>>>>>> Hi,
>>>>>>
>>>>>> thanks to everyone for looking at this.
>>>>>>
>>>>>>> My concern with this series of changes is that we're claiming to be
>>>>>>> setting shared posix values when it seems in fact the requirements
>>>>>>> are different across "posix" platforms. We now penalize all posix
>>>>>>> platforms with a larger minimum stack due to this Solaris x64 issue.
>>>>>>
>>>>>> Why do we penalize all platforms with this fix? The changes are to
>>>>>> 'os_solaris_x86.cpp' and only affect a Solaris x86 build.
>>>>>
>>>>> Sorry I was mis-remembering that despite being called
>>>>> os::Posix::_compiler_thread_min_stack_allowed it is actually a platform
>>>>> specific value.
>>>
>>> Okay, no problem.
>>>
>>> Thanks,
>>> Tobias
>>>
>>>>>
>>>>> David
>>>>>
>>>>>>> I'm also very curious as to why Solaris x64 needs to be different?
>>>>>>
>>>>>> [see below]
>>>>>>
>>>>>>>> Anyways, this size looks strange. Other platforms get along with 32K
>>>>>>>> and 48K
>>>>>>>> compiler thread size!  Why does this platform need ten times more
>>>>>>>> than others?
>>>>>>>> Is it because C2 optimizations are configured more aggressive,
>>>>>>>> leading to bigger
>>>>>>>> compile tasks during startup?
>>>>>>>> But solaris_sparc also needs a lot (102K) as well as aix (192K). At
>>>>>>>> some point
>>>>>>>> I'll have a look at why aix needs that much.
>>>>>>
>>>>>> I don't think it's due to C2 optimizations. They shouldn't differ much
>>>>>> on Solaris x86 compared to Linux x86.
>>>>>>
>>>>>> I had a closer look at the failures and we always fail in the C2
>>>>>> matcher in the generated method State::MachNodeGenerator(int)
>> which
>>>>>> basically consists of a *huge* switch statement matching opcodes.
>>>>>> Turns out that with a product build we reserve lots of stack space on
>>>>>> method entry:
>>>>>>
>>>>>> 0xffff80ff92537cf0<MachNode*State::MachNodeGenerator(int)>:
>> push
>>>>>> %rbp
>>>>>> 0xffff80ff92537cf1<MachNode*State::MachNodeGenerator(int)+1>:
>> mov
>>>>>> %rsp,%rbp
>>>>>> 0xffff80ff92537cf4<MachNode*State::MachNodeGenerator(int)+4>:
>> push
>>>>>> %r15
>>>>>> 0xffff80ff92537cf6<MachNode*State::MachNodeGenerator(int)+6>:
>> sub
>>>>>> $0x53ba8,%rsp
>>>>>>
>>>>>> This requires ~335k of stack space whereas a fastdebug build only
>>>>>> requires ~15k for the same method:
>>>>>>
>>>>>> 0xffff80ff7c8a0460<MachNode*State::MachNodeGenerator(int)>:
>> push
>>>>>> %rbp
>>>>>> 0xffff80ff7c8a0461<MachNode*State::MachNodeGenerator(int)+1>:
>> mov
>>>>>> %rsp,%rbp
>>>>>> 0xffff80ff7c8a0464<MachNode*State::MachNodeGenerator(int)+4>:
>> sub
>>>>>> $0x9430,%rsp
>>>>>>
>>>>>> This is consistent with my findings that a fastdebug VM requires a
>>>>>> CompilerThreadStackSize of 155k whereas a product build requires
>> 384k.
>>>>>>
>>>>>> I guess that this difference is due to more aggressive optimizations
>>>>>> done by the Sun Studio Compiler for product builds but from looking at
>>>>>> the assembly code, it's not obvious what all that space is actually
>>>>>> used for.
>>>>>>
>>>>>> Do you think we should file a Sun Studio Compiler bug for this?
>>>>>>
>>>>>>>> Basically this is the right fix for the immediate problem, though.
>>>>>>>> Looks good.
>>>>>>
>>>>>> Thanks, if there are no objections I would like to push this fix.
>>>>>>
>>>>>> Thanks,
>>>>>> Tobias
>>>>>>
>>>>>>>>
>>>>>>>> Best regards,
>>>>>>>>   Goetz
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>> -----Original Message-----
>>>>>>>>> From: Daniel D. Daugherty [mailto:daniel.daugherty at oracle.com]
>>>>>>>>> Sent: Samstag, 14. Januar 2017 01:04
>>>>>>>>> To: Tobias Hartmann <tobias.hartmann at oracle.com>; hotspot-
>>>>>>>>> dev at openjdk.java.net Developers <hotspot-
>> dev at openjdk.java.net>;
>>>>>>>>> Lindenmaier, Goetz <goetz.lindenmaier at sap.com>
>>>>>>>>> Subject: Re: [9] RFR(S): 8172731:
>>>>>>>>> runtime/Thread/TooSmallStackSize.java fails
>>>>>>>>> on solaris-x64 with product build
>>>>>>>>>
>>>>>>>>> Adding Goetz to this email thread directly.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 1/13/17 10:37 AM, Tobias Hartmann wrote:
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>> please review the following patch:
>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8172731
>>>>>>>>>> http://cr.openjdk.java.net/~thartmann/8172731/webrev.00/
>>>>>>>>>
>>>>>>>>> src/os_cpu/solaris_x86/vm/os_solaris_x86.cpp
>>>>>>>>>      No comments.
>>>>>>>>>
>>>>>>>>> Thumbs up! Thanks for fixing this issue.
>>>>>>>>>
>>>>>>>>> Goetz, can you please chime in on this thread since you were the
>>>>>>>>> author for:
>>>>>>>>>
>>>>>>>>>      JDK-8170655: [posix] Fix minimum stack size computations
>>>>>>>>>      https://bugs.openjdk.java.net/browse/JDK-8170655
>>>>>>>>>
>>>>>>>>> My in-house testing as sponsor didn't catch this issue and now
>>>>>>>>> I look more closely at your testing comment in the bug report,
>>>>>>>>> I don't see Solaris X64 listed.
>>>>>>>>>
>>>>>>>>> Dan
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> The test fails because the VM crashes with a
>>>>>>>>>> CompilerThreadStackSize of
>>>>>>>>> 300k. The problem is caused by the fix for JDK-8170655 [1] which
>>>>>>>>> now allows
>>>>>>>>> a minimum CompilerThreadStackSize of 300k, before it was 396k.
>> 300k
>>>>>>>>> is not
>>>>>>>>> enough to start the VM on Solaris x86. This didn't show up when
>> the
>>>>>>>>> fix was
>>>>>>>>> integrated because it was only tested with debug builds (with a
>>>>>>>>> debug build
>>>>>>>>> the number of StackShadowPages is increased by 2 and therefore
>> the
>>>>>>>>> minimal CompilerThreadStackSize is larger).
>>>>>>>>>>
>>>>>>>>>> Setting the guard pages to the minimum size via "-
>>>>>>>>> XX:StackShadowPages=10 -XX:StackRedPages=1 -
>> XX:StackYellowPages=2"
>>>>>>>>> requires a CompilerThreadStackSize of at least 381k to not crash at
>>>>>>>>> startup
>>>>>>>>> with a product build but the VM only checks for >= 260k. I
>>>>>>>>> increased the
>>>>>>>>> value of os::Posix::_compiler_thread_min_stack_allowed by 123 to
>>>>>>>>> 325 such
>>>>>>>>> that the minimum allowed CompilerThreadStackSize is 384k (due to
>> the
>>>>>>>>> additional space allocated for guard pages and rounding to the
>> page
>>>>>>>>> size).
>>>>>>>>>>
>>>>>>>>>> Tested with failing test, manually and with RBT (running).
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Tobias
>>>>>>>>>>
>>>>>>>>>> [1] https://bugs.openjdk.java.net/browse/JDK-8170655
>>>>>>>>