RFR: JDK-8193369: post_field_access does not work for some functions, possibly related to fast_getfield (original) (raw)

serguei.spitsyn at oracle.com serguei.spitsyn at oracle.com
Mon Mar 5 17:58:53 UTC 2018

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/fastfieldaccess/webrev.01/ See inline for comments for your notes. On 02/27/2018 23:08, serguei.spitsyn at 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/fastfieldaccess/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/fastfieldaccess/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: postfieldaccess does not work for some functions, possibly related to fastgetfield 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/fastfieldaccess/webrev/ --alex

More information about the serviceability-dev mailing list