[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR: JDK-8193369: post_field_access does not work for some functions, possibly related to fast_g
From:       "serguei.spitsyn () oracle ! com" <serguei ! spitsyn () oracle ! com>
Date:       2018-02-28 7:08:50
Message-ID: 91aadc35-125a-bf74-6cf5-672dc77ffb22 () oracle ! com
[Download RAW message or body]

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.

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?

I'm thinking if moving the test() to native side would simplify things.
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.

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?
You can find some tests with examples in the serviceability/jvmti folder:

static const char *EXC_CNAME = "java/lang/Exception";

static
jint throw_exc(JNIEnv *env, char *msg) {
        jclass exc_class = JNI_ENV_PTR(env)->FindClass(JNI_ENV_ARG(env, 
EXC_CNAME));

        if (exc_class == NULL) {
                printf("throw_exc: Error in FindClass(env, %s)\n", EXC_CNAME);
                return -1;
        }
        return JNI_ENV_PTR(env)->ThrowNew(JNI_ENV_ARG(env, exc_class), msg);
}


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.

  168                 (*jvmti)->Deallocate(jvmti, (unsigned char*)sig);

  The sig needs to be cleared after deallocation as it is used and checked in a loop.


Missed initializations:

   68     char *name;
  142         jfieldID* klassFields;
  143         jint fieldCount;


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


[prev in list] [next in list] [prev in thread] [next in thread]