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.



On 3/8/18 1:48 PM, serguei.spit...@oracle.com wrote:
Hey guys,

One more review is needed for this fix!


On 3/5/18 09:58, serguei.spit...@oracle.com wrote:
Hi Alex,

It looks good.
Thank you for the update!


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.

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.

Probably, the intention was to spell "startTest" insted of "initTest" below:

  119         if (!startTest(result)) {
  120             throw new RuntimeException("initTest failed");
  121         }

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.

I'd suggest to rename currentTestResults to testResultObject,
so it will be in line with testResultClass.

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;



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/


Reply via email to