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

Wed Nov 20 20:54:05 UTC 2019

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 <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