RFR(L): JDK-8046936 : JEP 270: Reserved Stack Areas for Critical Sections

[Karen Kinnear]

Frederic,

Looks good.

Many thanks.
Karen

> On Nov 23, 2015, at 12:44 PM, Frederic Parain <frederic.parain at oracle.com> wrote:
> 
> Karen,
> 
> Thank you for your review, my answers are in-lined below.
> 
> New Webrevs (including some fixes suggested by Paul Sandoz):
> 
> http://cr.openjdk.java.net/~fparain/8046936/webrev.01/hotspot/ <http://cr.openjdk.java.net/~fparain/8046936/webrev.01/hotspot/>
> http://cr.openjdk.java.net/~fparain/8046936/webrev.01/jdk/ <http://cr.openjdk.java.net/~fparain/8046936/webrev.01/jdk/>
> 
> On 20/11/2015 19:44, Karen Kinnear wrote:
>> Frederic,
>> 
>> Code review for web revs you sent out.
>> Code looks good. I am not as familiar with the compiler code.
>> 
>> I realize you need to check in at least a subset of the java.util.concurrent changes for
>> the test to work, so maybe I should not have asked Doug about his preference there.
>> Sorry.
>> 
>> I am impressed you found a way to make a reproducible test!
>> 
>> Comments/questions:
>> 1. src/cpu/sparc/vm/interp_masm_sparc.cpp line 1144 “is” -> “if”
> 
> Fixed
> 
>> 2. IIRC, due to another bug with windows handling of our guard pages, this
>> is disabled for Windows. Would it be worth putting a comment in the
>> bug , 8067946, that once this is fixed, the reserved stack logic on windows
>> will need testing before enabling?
> 
> More than testing, the implementation would have to be completed on
> Windows. I've added a comment to JDK-8067946.
> 
>> 3. In get_frame_at_stack_banging_point on Linux, BSD and solaris_x86 if
>> this is in interpreter code, you explicitly return the Java sender
>> of the current frame. I was expecting to see that on Solaris_sparc and Windows
>> as well? I do see the assertion in caller that you do have a java frame.
> 
> It doesn't make sense to check the current frame (no bytecode has been
> executed yet, so risk of partially executed critical section). I did the
> change but not for all platforms. This is now fixed for Solaris_SPARC
> and Windows too.
> 
>> 4. test line 83 “writtent” -> “written”
> 
> Fixed
> 
>> 5. I like the way you set up the preallocated exception and then set the message - we may
>> try that for default methods in future.
>> 
>> 6. I had a memory that you had found a bug in safe_for_sender - did you already check that in?
> 
> I've fixed x86 platforms in JDK-8068655.
> I've piggybacked the SPARC fix to this webrev (frame_sparc.cpp:635),
> I had hoped it would have been silently accepted :-)
> 
> 
>> 7. I see the change in trace.xml, and I see an include added to SharedRuntime.cpp,
>> but I didn’t see where it was used - did I just miss it?
> 
> trace.xml changes define a new event.
> This event is created at sharedRuntime.cpp::3006 and it is used
> in the next 3 lines.
Thanks. I must have mistyped when I searched for it.
> 
> Thanks,
> 
> Fred
> 
> -- 
> Frederic Parain - Oracle
> Grenoble Engineering Center - France
> Phone: +33 4 76 18 81 17
> Email: Frederic.Parain at oracle.com <mailto:Frederic.Parain at oracle.com>