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

Mon Nov 25 16:27:46 UTC 2019

Looks good.

Erik

On 2019-11-20 21:54, Markus Gronlund wrote:
>
> "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 <markus.gronlund at oracle.com>; hotspot-jfr-dev 
> <hotspot-jfr-dev at openjdk.java.net>; 
> hotspot-runtime-dev at openjdk.java.net; serviceability-dev at openjdk.java.net
> *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
>