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

[Frederic Parain]

Karen,

Thank you for your review.

Fred

On 23/11/2015 20:10, Karen Kinnear wrote:
> Frederic,
>
> Looks good.
>
> Many thanks.
> Karen
>
>> On Nov 23, 2015, at 12:44 PM, Frederic Parain
>> <frederic.parain at oracle.com <mailto: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/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>
>

-- 
Frederic Parain - Oracle
Grenoble Engineering Center - France
Phone: +33 4 76 18 81 17
Email: Frederic.Parain at oracle.com