Hi Serguei, thanks for taking a look.
"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." Yes, so the reason for the phase transition is not so much to do with capabilities, but that an agent can only register, i.e. call GetEnv(), in phases JVMTI_PHASE_ONLOAD and JVMTI_PHASE_LIVE. create_vm_init_agents() is where the (temporary) JVMTI_PHASE_PRIMORDIAL to JVMTI_PHASE_ONLOAD happens during the callouts to Agent_OnLoad(), and then the state is returned to JVMTI_PHASE_PRIMORDIAL. It is hard to find an unconditional hook point there since create_vm_init_agents() is made conditional on Arguments::init_agents_at_startup(), with a listing populated from "real agents" (on command-line). The JFR JVMTI agent itself is also conditional, installed only if JFR is actively started (i.e. a starting a recording). Hence, the phase transition mechanism merely replicates the state changes in create_vm_init_agents() to have the agent register properly. This is a moot point now however as I have taken another pass. I now found a way to only have the agent register during the JVMTI_PHASE_LIVE phase, so the phase transition mechanism is not needed. "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." Yes, this is confusing, I agree. Of course, JFR has a tight relation to the JVMTI phases, but only in so far as to coordinate agent registration. The JFR calls are not intended to reflect the JVMTI phases per se but a more general initialization order state description, like you say "VM initialization start and completion". However, it is very hard to encode proper semantics into the JFR calls in Threads::create_vm() to reflect the concepts of "stages"; they are simply not well-defined. In addition, there are so many of them J. For example, I always get confused that VM initialization is reflected in JVMTI by the VMStart event and the completion by the VMInit event (representing VM initialization complete). At the same time, the DTRACE macros have both HOTSPOT_VM_INIT_BEGIN() HOTSPOT_VM_INIT_END() placed before both... I abandoned the attempt to encode anything meaningful into the JFR calls trying to represent a certain "VM initialization stage". Instead, I will just have syntactic JFR calls reflecting some relative order (on_create_vm_1(), on_create_vm_2(),.. _3()) etc. Looks like there are precedents of this style. “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 for the suggestion Serguei, but these capabilities are not yet needed. Here is the updated webrev: http://cr.openjdk.java.net/~mgronlun/8233197/webrev02/ Thanks again Markus From: Serguei Spitsyn Sent: den 20 november 2019 04:10 To: Markus Gronlund <[email protected]>; hotspot-jfr-dev <[email protected]>; [email protected]; [email protected] Subject: Re: 8233197(S): Invert JvmtiExport::post_vm_initialized() and Jfr:on_vm_start() start-up order for correct option parsing 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
