8233197(S): Invert JvmtiExport::post_vm_initialized() and Jfr:on_vm_start() start-up order for correct option parsing
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Thu Nov 21 00:52:41 UTC 2019
Hi Marcus,
Thank you for the answers!
The update looks good to me.
A couple of minor minor comments.
http://cr.openjdk.java.net/~mgronlun/8233197/webrev02/src/hotspot/share/jfr/instrumentation/jfrJvmtiAgent.cpp.frames.html
57 static bool set_event_notification_mode(jvmtiEventMode mode,
58 jvmtiEvent event,
59 jthread event_thread,
60 ...) {
You may want to align arguments.
126 size_t length = sizeof base_error_msg ; // includes terminating null
Unneeded space before ';'.
Would it better to use this form: sizeof(base_error_msg)?
No need in another webrev.
Thanks,
Serguei
On 11/20/19 12:54 PM, Markus Gronlund wrote:
>
> 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
>
More information about the hotspot-runtime-dev
mailing list