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

Tue Jan 17 03:51:46 UTC 2017

On 1/16/17 5:53 PM, David Holmes wrote:
> On 17/01/2017 12:26 AM, Tobias Hartmann wrote:
>> 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?
>
> Probably should say "Solaris Studio" rather than "Sun"

It would also be good to document that the fastdebug version
requires less stack space than the release version. That bit
of strangeness should be called out clearly...

I'm good with pushing this fix.

Dan

>
> Thanks,
> David
>
>> 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
>>>>>>>>