[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: PING: Re: RFR: JDK-8193369: post_field_access does not work for some functions, possibly related
From:       Chris Plummer <chris.plummer () oracle ! com>
Date:       2018-03-12 16:52:15
Message-ID: 077a8d3f-badf-fd68-b889-5604d4340891 () oracle ! com
[Download RAW message or body]

Hi Alex,

Please update the copyright date in jvmtiManageCapabilities.cpp.

The following is where you added your fix:

   315     if (avail.can_generate_breakpoint_events
   316               || avail.can_generate_field_access_events
   317               || avail.can_generate_field_modification_events)
   318     {
   319         RewriteFrequentPairs = false;
   320     }

Although this addresses the problem, in general I think this approach is 
error prone since it requires knowledge of which bytecode pairs might be 
rewritten, and the impact they may have on JVMTI. But that's a 
pre-existing issue with this code, not something I'd expect you to fix 
with this CR, so looks good.

The test case also looks good.

thanks,

Chris

On 3/8/18 1:48 PM, serguei.spitsyn@oracle.com wrote:
> Hey guys,
> 
> One more review is needed for this fix!
> 
> Thanks,
> Serguei
> 
> 
> On 3/5/18 09:58, serguei.spitsyn@oracle.com wrote:
> > Hi Alex,
> > 
> > It looks good.
> > Thank you for the update!
> > 
> > Thanks,
> > Serguei
> > 
> > On 3/1/18 10:53, Alex Menkov wrote:
> > > Hi Serguei,
> > > 
> > > Thank you for the feedback.
> > > Updated webrev: 
> > > http://cr.openjdk.java.net/~amenkov/fast_field_access/webrev.01/
> > > 
> > > See inline for comments for your notes.
> > > 
> > > On 02/27/2018 23:08, serguei.spitsyn@oracle.com wrote:
> > > > Hi Alex,
> > > > 
> > > > Thank you for taking care about this!
> > > > The fix looks good to me.
> > > > 
> > > > Some comments on the test.
> > > > 
> > > > http://cr.openjdk.java.net/~amenkov/fast_field_access/webrev/test/hotspot/jtreg/serviceability/jvmti/FieldAccessWatch/FieldAccessWatch.java.html \
> > > >  
> > > > There are some commented lines in the TestResult class.
> > > > A cleanup is needed to delete them.
> > > > I guess, it is already in your plan. 
> > > 
> > > I deleted couple lines, keeping comment for fields
> > > > The empty line #135 is not needed.
> > > > An empty line is needed after the L99. 
> > > 
> > > fixed.
> > > > Probably, the intention was to spell "startTest" insted of 
> > > > "initTest" below:
> > > > 
> > > > 119                 if (!startTest(result)) {
> > > > 120                         throw new RuntimeException("initTest failed");
> > > > 121                 } 
> > > 
> > > fixed.
> > > > I wonder if this sleep is really needed:
> > > > 124 Thread.sleep(500);
> > > > 
> > > > The "action.apply()" is executed synchronously, is not it? 
> > > 
> > > But notifications are asynchronous, so this helps to avoid test 
> > > failures is some events are delivered a bit later in loaded 
> > > environment.
> > > Also this helps to avoid mess of native and java logging
> > > > I'm thinking if moving the test() to native side would simplify 
> > > > things. 
> > > 
> > > To me it's simpler and more flexible to perform required actions in 
> > > Java, native part only handles notifications.
> > > > An Exception can be thrown from native if the test failed or just a 
> > > > boolean status returned.
> > > > 
> > > > 
> > > > http://cr.openjdk.java.net/~amenkov/fast_field_access/webrev/test/hotspot/jtreg/serviceability/jvmti/FieldAccessWatch/libFieldAccessWatch.c.html \
> > > >  
> > > > I'd suggest to rename currentTestResults to testResultObject,
> > > > so it will be in line with testResultClass. 
> > > 
> > > fixed.
> > > > One concern is that that the reportError() does not cause the test 
> > > > to fail and does not break the execution.
> > > > Would it better to throw an exception with the same message as was 
> > > > printed? 
> > > 
> > > Updated several cases (immediate return from callbacks if something 
> > > went wrong).
> > > Note that reportError is called from native Java methods and from 
> > > JVMTI callbacks, so throwing an exception doesn't looks right.
> > > > It seems, the function tagAndWatch() adds some complexity to the code.
> > > > Is all this really needed? Could you, please, add some comments.
> > > > It does not seem this functions tags anything. 
> > > 
> > > renamed the function, added short function description.
> > > > 168 (*jvmti)->Deallocate(jvmti, (unsigned char*)sig);
> > > > 
> > > > The sig needs to be cleared after deallocation as it is used and 
> > > > checked in a loop. 
> > > 
> > > Moved the variable to the correct scope.
> > > > Missed initializations:
> > > > 
> > > > 68         char *name;
> > > > 142                 jfieldID* klassFields;
> > > > 143                 jint fieldCount; 
> > > 
> > > Fixed.
> > > 
> > > --alex
> > > > Thanks,
> > > > Serguei
> > > > 
> > > > 
> > > > On 2/26/18 14:43, Alex Menkov wrote:
> > > > > Hi all,
> > > > > 
> > > > > Please review a fix for
> > > > > JDK-8193369: post_field_access does not work for some functions, 
> > > > > possibly related to fast_getfield
> > > > > 
> > > > > The fix disables "fast" command generation when FieldAccess or 
> > > > > FieldModification notifications are requested.
> > > > > 
> > > > > jira: https://bugs.openjdk.java.net/browse/JDK-8193369
> > > > > webrev: http://cr.openjdk.java.net/~amenkov/fast_field_access/webrev/
> > > > > 
> > > > > --alex 
> 


[prev in list] [next in list] [prev in thread] [next in thread]