Frederic, Looks good.
Many thanks. Karen > On Nov 23, 2015, at 12:44 PM, Frederic Parain <frederic.par...@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.par...@oracle.com <mailto:frederic.par...@oracle.com>
