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.spit...@oracle.com wrote:
Hey guys,
One more review is needed for this fix!
Thanks,
Serguei
On 3/5/18 09:58, serguei.spit...@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.spit...@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
