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

[serguei.spitsyn at oracle.com]

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