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.gronl...@oracle.com>; hotspot-jfr-dev
<hotspot-jfr-...@openjdk.java.net>;
hotspot-runtime-...@openjdk.java.net; serviceability-dev@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