Re: 8233197(S): Invert JvmtiExport::post_vm_initialized() and Jfr:on_vm_start() start-up order for correct option parsing

Tue, 19 Nov 2019 19:13:22 -0800

Hi Marcus,

It looks good in general.

A couple of comments though.

http://cr.openjdk.java.net/~mgronlun/8233197/webrev01/src/hotspot/share/jfr/instrumentation/jfrJvmtiAgent.cpp.frames.html
 258 class JvmtiPhaseTransition {
 259  private:
 260   bool _transition;
 261  public:
 262   JvmtiPhaseTransition() : _transition(JvmtiEnvBase::get_phase() == JVMTI_PHASE_PRIMORDIAL) {
 263     if (_transition) {
 264       JvmtiEnvBase::set_phase(JVMTI_PHASE_ONLOAD);
 265     }
 266   }
 267   ~JvmtiPhaseTransition() {
 268     if (_transition) {
 269       assert(JvmtiEnvBase::get_phase() == JVMTI_PHASE_ONLOAD, "invariant");
 270       JvmtiEnvBase::set_phase(JVMTI_PHASE_PRIMORDIAL);
 271     }
 272   }
 273 };
 274 
 275 static bool initialize() {
 276   JavaThread* const jt = current_java_thread();
 277   assert(jt != NULL, "invariant");
 278   assert(jt->thread_state() == _thread_in_vm, "invariant");
 279   DEBUG_ONLY(JfrJavaSupport::check_java_thread_in_vm(jt));
 280   JvmtiPhaseTransition jvmti_phase_transition;
 281   ThreadToNativeFromVM transition(jt);
 282   if (create_jvmti_env(jt) != JNI_OK) {
 283     assert(jfr_jvmti_env == NULL, "invariant");
 284     return false;
 285   }
 286   assert(jfr_jvmti_env != NULL, "invariant");
 287   if (!register_capabilities(jt)) {
 288     return false;
 289   }
 290   if (!register_callbacks(jt)) {
 291     return false;
 292   }
 293   return update_class_file_load_hook_event(JVMTI_ENABLE);
 294 }

It does not look as a good idea to change the JVMTI phase like above.
If you need the ONLOAD phase just to enable capabilities then it is better to do it in the real ONLOAD phase.
Do I miss anything important here?
Please, ask questions if you have any problems with it.

The Jfr::on_vm_init() is confusing as there is a mismatch with the JVMTI phases order.
It fills like it means JFR init event (not VM init) or something like this.
Or maybe it denotes the VM initialization start. :)
I'll be happy if you could explain it a little bit.

Not sure, if your agent needs to enable these capabilities (introduced in JDK 9 with modules):
  can_generate_early_vmstart
  can_generate_early_class_hook_events

Thanks,
Serguei


On 11/19/19 06:38, Markus Gronlund wrote:
Greetings,

(apologies for the wide distribution)

Kindly asking for reviews for the following changeset:

Bug: https://bugs.openjdk.java.net/browse/JDK-8233197 
Webrev: http://cr.openjdk.java.net/~mgronlun/8233197/webrev01/
Testing: serviceability/jvmti, jdk_jfr, tier1-5
Summary: please see bug for description.

For Runtime / Serviceability folks:
This change slightly modifies the relative order in Threads::create_vm(); please see threads.cpp.
There is an upcall as part of Jfr::on_vm_start() that delivers global JFR command-line options to Java (only if set).
The behavioral change amounts to a few classes loaded as part of establishing this upcall (all internal JFR classes and/or java.base classes, loaded by the bootloader) no longer being visible to the ClassFileLoadHook's of agents. These classes are visible to agents that work with "early_start" JVMTI environments however.

The major part of JFR startup with associated class loading still happens as part of Jfr::on_vm_live() with no behavioral change in relation to agents.

Thank you
Markus

Reply via email to