ARM: Support for JVMTI notifications from JIT compiler on code generation

[Andrew Dinn]

On 25/04/12 14:28, Andrew Haley wrote:
>> I have asked Andrew Haley to review this patch.
> 
> But if anyone else wants to join in...

Of course, all feedback is welcome.

>> diff -r a5d04cb60a5c -r fada6a7d5e89 src/cpu/zero/vm/thumb2.cpp
>> --- a/src/cpu/zero/vm/thumb2.cpp	Wed Apr 11 09:24:03 2012 -0400
>> +++ b/src/cpu/zero/vm/thumb2.cpp	Wed Apr 25 13:43:33 2012 +0100
>> @@ -440,6 +440,12 @@
>>  unsigned stack[1000];
>>  unsigned r_local[1000];
>>  
>> +// jvmti needs to map start address of generated code for a bytecode
>> +// to corrsponding bytecode index so agents can correlate code address
>> +// ranges with bci and thence line number
>> +jvmtiAddrLocationMap address_bci_map[THUMB2_MAX_BYTECODE_SIZE];
> 
> I presume that there need only be one of these because it is only
> accessed under lock.  However, it's 160kbytes in size.  It
> might be a better idea to allocate the space at runtime: not every
> VM runs the JIT.

Yes, will do so.

>>    jinfo_str.code_base = code_base;
>> @@ -7580,6 +7597,27 @@
>>    if (compiled_offset == 0) return 0;
>>    thumb_entry.compiled_entrypoint = slow_entry + compiled_offset;
>>    thumb_entry.osr_entry = (unsigned)cmethod->osr_entry | TBIT;
>> +  {
>> +    // we need to notify a Jvmti compiled_method_load event
>> +
>> +    // notify the whole generated code region for this Java method
>> +    // from slow_entry through to the end of the osr table. some
>> +    // of it is data not code but that's not a problem.
> 
> I have a personal preference for sentences in comments.

By my reading the opening 2 lines of the second comment /are/ a sentence
(with a verb in the imperative mood). of course, your mileage (and
grammar) may vary.

I am happy to add a full stop after the sentence in the first comment
and some ','s to place  the 'not code' properly in apposition if you like.

>> +
>> +    const void *gen_code_start = (const void *)(slow_entry ^ TBIT);
>> +    unsigned gen_code_size = codebuf_str.idx * 2;
>> +
>> +    // address_bci_map translates start addresses for generated code
>> +    // sections to bytecode indices
>> +
>> +    // the final compile_info argument is supposed to contain
>> +    // information about inlined code. we can supply NULL for now -
>> +    // oprofile doesn't use it anyway
>> +
>> +    void *compile_info = NULL;
>> +
>> +    JvmtiExport::post_compiled_method_load(method, gen_code_size, gen_code_start, address_bci_map_length, address_bci_map, NULL);
> 
> Please avoid lines so long that they wrap.

Indeed.

>> +  }
>>    return *(unsigned long long *)&thumb_entry;
>>  }
>>  
>> diff -r a5d04cb60a5c -r fada6a7d5e89 src/share/vm/prims/jvmtiExport.cpp
>> --- a/src/share/vm/prims/jvmtiExport.cpp	Wed Apr 11 09:24:03 2012 -0400
>> +++ b/src/share/vm/prims/jvmtiExport.cpp	Wed Apr 25 13:43:33 2012 +0100
>> @@ -1776,6 +1776,48 @@
>>    }
>>  }
>>  
>> +#ifdef __arm__
>> +
>> +// special compiled_method_load notify API for thumb2 compiler
>> +
>> +void JvmtiExport::post_compiled_method_load(const methodOop method, const jint length,
>> +                                            const void *code_begin, const jint map_length,
>> +                                            const jvmtiAddrLocationMap* map,
>> +					    const void *compile_info)
>> +{
>> +  JavaThread* thread = JavaThread::current();
>> +  jmethodID methodId = method->jmethod_id();
>> +
>> +  EVT_TRIG_TRACE(JVMTI_EVENT_COMPILED_METHOD_LOAD,
>> +                 ("JVMTI [%s] method compile load event triggered (by thumb2_compile)",
>> +                 JvmtiTrace::safe_get_thread_name(thread)));
>> +
>> +  JvmtiEnvIterator it;
>> +  for (JvmtiEnv* env = it.first(); env != NULL; env = it.next(env)) {
>> +    if (env->is_enabled(JVMTI_EVENT_COMPILED_METHOD_LOAD)) {
>> +
>> +      EVT_TRACE(JVMTI_EVENT_COMPILED_METHOD_LOAD,
>> +                ("JVMTI [%s] class compile method load event sent %s.%s   (by thumb2_compile)",
>> +                JvmtiTrace::safe_get_thread_name(thread),
>> +                method->klass_name()->as_C_string(),
>> +		 method->name()->as_C_string()));
>> +
>> +      // I think we need this to add a resource mark and handle for
>> +      // the thread and record a vm to native (agent lib) transition
>> +
>> +      JvmtiJavaThreadEventTransition jet(thread);
>> +      jvmtiEventCompiledMethodLoad callback = env->callbacks()->CompiledMethodLoad;
>> +
>> +      if (callback != NULL) {
>> +        (*callback)(env->jvmti_external(), methodId,
>> +                    length, code_begin, map_length,
>> +                    map, compile_info);
>> +      }
>> +    }
>> +  }
>> +}
>> +
>> +#endif // __arm__
> 
> This is a bit unfortunate.  I presume we have to do it because JvmtiExport
> doesn't give us what we need.

Yes, the client API method provided for the other compilers expects an
nmethod instance so using it would drag us into creating nmethods which
means supporting the full AbstractCompiler lifecycle.

There is another API method which is provided for use if you already
have a handle on a JvmtiEnv -- not sure where it is actually used but I
think it is intended for agent implementations to use. Using it would
require the ARM JIT code to do the iteration over the JvmtiEnv list and
I regarded that as something which ought to be kept internal to
JvmtiExport. Hence the invention of this new API method.

I am not certain about the value of the declared
JvmtiJavaThreadEventTransition. I am assuming that it is needd to retain
a handle on the thread when we enter into JVMI agent code in case of
either GCs or exceptions. Perhaps someone can comment on whether this is
i) necessary and ii) sufficient (e.g. do we also need to declare a
HandleMark for the methodOop?).

regards,


Andrew Dinn
-----------