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

[Alex Menkov]

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
>