RFR(S): 8017313: PPC64 (part 6): cppInterpreter: stack handling improvements

Wed Jun 26 14:55:31 PDT 2013

On 6/26/2013 5:25 PM, Vladimir Kozlov wrote:
> I updated bug report to remove cppInterpreter from synopsis and 
> description.
>
> Do we have agreement on these changes?

Also, I think these changes look good.

coleen
>
> Will runtime group do changes in our code to use new functionality?
> If not, we should push it into staging repo because I don't see other 
> reasons to push it into main sources now.
>
> Thanks,
> Vladimir
>
> On 6/26/13 6:28 AM, Lindenmaier, Goetz wrote:
>> Hi,
>>
>> I added shared initialization code of the new field.
>> I also moved the code from thread to JavaThread, as only
>> there overflow checks happen.
>> http://cr.openjdk.java.net/~goetz/webrevs/8017313-stack-2/
>>
>> Do you want to push this to some main repository (if you want
>> to use this on other platforms) or should I push it into the
>> ppc64 staging repo once tested and finally reviewed?
>>
>> Best regards,
>>    Goetz.
>>
>> From: Coleen Phillimore [mailto:coleen.phillimore at oracle.com]
>> Sent: Dienstag, 25. Juni 2013 16:23
>> To: Lindenmaier, Goetz
>> Cc: hotspot-dev at openjdk.java.net; David Holmes
>> Subject: Re: RFR(S): 8017313: PPC64 (part 6): cppInterpreter: stack 
>> handling improvements
>>
>>
>> On 06/25/2013 10:07 AM, Lindenmaier, Goetz wrote:
>> Hi,
>>
>> that's something I don't get:  x86 does
>>    // Use the maximum number of pages we might bang.
>>    const int max_pages = StackShadowPages > 
>> (StackRedPages+StackYellowPages) ? StackShadowPages :
>> (StackRedPages+StackYellowPages)
>> and sparc
>>      __ set( (StackRedPages+StackYellowPages) * page_size, Rscratch2 );
>> in generate_stack_overflow_check().
>>
>> I don't know why these are different, except that they are coded 
>> separately and probably last touched at different times.
>>
>>
>>
>> And we use the sum of all three.  This is because we want to use the
>> shadow space for exception handling, so we must overflow before we
>> run into the shadow space.
>>
>> Yes, we don't want to check that we are not pushing stack frames into 
>> the shadow space here.
>>
>>
>>
>> All these different values could be set in the os_cpu file.
>> But maybe they only differ because there is no shared coding
>> yet?  And they should be harmonized?
>>
>> They absolutely should be harmonized, which would  be good if the 
>> calculation that it uses is shared and the assembly code is 
>> simplified.   We can do that after your change.
>>
>>
>>
>> Bertrand, recomputing stack_base seems a good idea to me.
>> But this would require that all platforms are adapted before
>> _stack_base is removed.
>>
>> We could do that when we fix the other platforms to use 
>> stack_overflow_limit().   The extra field in Thread is worth it.   
>> There are a lot of fields in Thread that we can remove (our 
>> out-pointer because they're never used) instead.
>>
>> Coleen
>>
>>
>>
>> Best regards,
>> Goetz.
>>
>>
>>
>> From: Coleen Phillimore [mailto:coleen.phillimore at oracle.com]
>> Sent: Dienstag, 25. Juni 2013 15:34
>> To: Lindenmaier, Goetz
>> Cc: 
>> hotspot-dev at openjdk.java.net<mailto:hotspot-dev at openjdk.java.net>; 
>> David Holmes
>> Subject: Re: RFR(S): 8017313: PPC64 (part 6): cppInterpreter: stack 
>> handling improvements
>>
>>
>> Yes, I think this would be useful for the other platforms to simplify 
>> the stack overflow limit checking in the interpreter assembly code.
>>
>>        100   // Initialize the memory stack limit.
>>
>>        101   address mem_stk_limit = thread->stack_base() - 
>> thread->stack_size() +
>>
>>        102     ((StackShadowPages + StackYellowPages + StackRedPages) 
>> * os::vm_page_size());
>>
>>        103
>>
>>        104   thread->set_memory_stack_limit(mem_stk_limit);
>>
>>
>>
>>
>> My other comment was to move 101-102 into the function 
>> set_stack_overflow_limit() since nothing on those lines depends on 
>> the platform and the values used to compute this are owned by class 
>> Thread anyway.
>>
>> thanks,
>> Coleen
>> On 06/25/2013 09:27 AM, Lindenmaier, Goetz wrote:
>>
>> Hi Colleen, David,
>>
>>
>>
>> I'll rename the methods to stack_overflow_limit and update the webrev,
>>
>> I like that name better too.
>>
>>
>>
>> We set it in os::initialize_thread():
>>
>> http://hg.openjdk.java.net/ppc-aix-port/jdk8/hotspot/file/2a6fd169b0b7/src/os_cpu/linux_ppc/vm/os_linux_ppc.cpp 
>>
>>
>> and we use it here (line427):
>>
>> http://hg.openjdk.java.net/ppc-aix-port/jdk8/hotspot/file/2a6fd169b0b7/src/cpu/ppc/vm/cppInterpreter_ppc.cpp 
>>
>>
>> In that files the method is called [set_]memory_stack_limit().
>>
>>
>>
>> Actually, it's got nothing to do with the cppInterpreter.  I first 
>> thought so,
>>
>> because we only used it with the cppInterpreter when I moved this from
>>
>> our VM to the port (a while ago now).  But look at the code in
>>
>> void InterpreterGenerator::generate_stack_overflow_check(void) on x86 
>> that
>>
>> computes the stack bound in rax ... that's not really simple, and 
>> requires
>>
>> two loads (stack_base, stack_size).
>>
>>
>>
>> The difference between cppInterpreter and template interpreter is that
>>
>> we always check against the stack_limit in the cppInterpreter, but the
>>
>> templateInterpreter first checks whether the stack is < 1 page, thus
>>
>> experiencing less overhead from the lengthy coding.
>>
>>
>>
>> Best regards,
>>
>>    Goetz
>>
>>
>>
>>
>>
>> -----Original Message-----
>>
>> From: 
>> hotspot-dev-bounces at openjdk.java.net<mailto:hotspot-dev-bounces at openjdk.java.net> 
>> [mailto:hotspot-dev-bounces at openjdk.java.net] On Behalf Of Coleen 
>> Phillimore
>>
>> Sent: Dienstag, 25. Juni 2013 14:49
>>
>> To: hotspot-dev at openjdk.java.net<mailto:hotspot-dev at openjdk.java.net>
>>
>> Subject: Re: RFR(S): 8017313: PPC64 (part 6): cppInterpreter: stack 
>> handling improvements
>>
>>
>>
>>
>>
>> Which stack limit is this?   Does it subtract off the red and yellow
>>
>> zones?   Where do you set it?  Yes, it would simplify the assembler code
>>
>> in the interpreter entries to use this.
>>
>>
>>
>> It might be better to rename it though if it's the usable stack
>>
>> (subtracting red and yellow zones) and maybe compute the value inside of
>>
>> Thread class (so you see it always subtracting the red and yellow
>>
>> zones).   It's sort of ambiguous that this is happening. How about
>>
>> stack_overflow_limit()?
>>
>>
>>
>> Thanks,
>>
>> Coleen
>>
>>
>>
>> On 06/25/2013 08:44 AM, Lindenmaier, Goetz wrote:
>>
>> Hi,
>>
>>
>>
>> We precompute the stack limit for overflow checks and save this
>>
>> limit in the thread.  This simplifies the assembler coding checking
>>
>> the stack overflow. This change contains the necessary shared
>>
>> functionality. It can easily be ported to other platforms, too.
>>
>> http://cr.openjdk.java.net/~goetz/webrevs/8017313-stack/<http://cr.openjdk.java.net/%7Egoetz/webrevs/8017313-stack/> 
>>
>>
>>
>>
>> Please review this change.
>>
>>
>>
>> Best regards,
>>
>>     Goetz.
>>
>>
>>
>> PS: I removed some parts of patch 0006 because I found a better
>>
>> platform-only solution.
>>
>>
>>
>>