PING: Re: RFR: JDK-8193369: post_field_access does not work for some functions, possibly related to fast_getfield

[Chris Plummer]

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