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

[serguei.spitsyn at oracle.com]

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
>>