RFR(S): 8017313: PPC64 (part 6): cppInterpreter: stack handling improvements
Lindenmaier, Goetz
goetz.lindenmaier at sap.com
Thu Jun 27 00:44:40 PDT 2013
Hi,
Coleen, Vladimir, David thanks for the reviews.
I updated the code style and added reviewers in the webrev.
http://cr.openjdk.java.net/~goetz/webrevs/8017313-stack-2/
Vladimir, will you run it through JPRT and push it to the repo?
I think it was not yet tested by you.
Best regards,
Goetz
-----Original Message-----
From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
Sent: Donnerstag, 27. Juni 2013 00:08
To: coleen.phillimore at oracle.com
Cc: Lindenmaier, Goetz; ppc-aix-port-dev at openjdk.java.net; David Holmes; hotspot-dev at openjdk.java.net
Subject: Re: RFR(S): 8017313: PPC64 (part 6): cppInterpreter: stack handling improvements
I am fine with changes, I have only code style comments.
Please, add parenthesis:
+ if (is_Java_thread()) {
+ ((JavaThread*) this)->set_stack_overflow_limit();
+ }
Add indention/spaces and split line in set_stack_overflow_limit() for
more clear view:
+ address stack_overflow_limit() { return _stack_overflow_limit; }
+ void set_stack_overflow_limit() {
+ _stack_overflow_limit = _stack_base - _stack_size +
+ ((StackShadowPages +
+ StackYellowPages +
+ StackRedPages) * os::vm_page_size());
+ }
Thanks,
Vladimir
On 6/26/13 2:55 PM, Coleen Phillimore wrote:
> 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.
>>>
>>>
>>>
>>>
>
More information about the ppc-aix-port-dev
mailing list