[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: 8233197(S): Invert JvmtiExport::post_vm_initialized() and Jfr:on_vm_start() start-up order for c
From: Erik Gahlin <erik.gahlin () oracle ! com>
Date: 2019-11-25 16:27:46
Message-ID: 8da1145a-5e65-8db8-8a11-9bce1af22233 () oracle ! com
[Download RAW message or body]
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@oracle.com>; hotspot-jfr-dev
> <hotspot-jfr-dev@openjdk.java.net>;
> hotspot-runtime-dev@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
>
[Attachment #3 (text/html)]
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<p>Looks good.</p>
<p>Erik</p>
<div class="moz-cite-prefix">On 2019-11-20 21:54, Markus Gronlund
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:62407a3d-f6a2-400b-9311-9ab7e32d85f7@default">
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
<meta name="Generator" content="Microsoft Word 15 (filtered
medium)">
<style><!--
/* Font Definitions */
@font-face
{font-family:Wingdings;
panose-1:5 0 0 0 0 0 0 0 0 0;}
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
{font-family:Consolas;
panose-1:2 11 6 9 2 2 4 3 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0cm;
margin-bottom:.0001pt;
font-size:11.0pt;
font-family:"Calibri",sans-serif;
color:black;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:purple;
text-decoration:underline;}
pre
{mso-style-priority:99;
mso-style-link:"HTML Preformatted Char";
margin:0cm;
margin-bottom:.0001pt;
font-size:10.0pt;
font-family:"Courier New";
color:black;}
p.msonormal0, li.msonormal0, div.msonormal0
{mso-style-name:msonormal;
mso-margin-top-alt:auto;
margin-right:0cm;
mso-margin-bottom-alt:auto;
margin-left:0cm;
font-size:11.0pt;
font-family:"Calibri",sans-serif;
color:black;}
span.HTMLPreformattedChar
{mso-style-name:"HTML Preformatted Char";
mso-style-priority:99;
mso-style-link:"HTML Preformatted";
font-family:"Consolas",serif;
color:black;}
span.new
{mso-style-name:new;}
span.changed
{mso-style-name:changed;}
span.EmailStyle22
{mso-style-type:personal-reply;
font-family:"Calibri",sans-serif;
color:windowtext;}
.MsoChpDefault
{mso-style-type:export-only;
font-size:10.0pt;}
@page WordSection1
{size:612.0pt 792.0pt;
margin:70.85pt 70.85pt 70.85pt 70.85pt;}
div.WordSection1
{page:WordSection1;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]-->
<div class="WordSection1"><o:p></o:p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">"It does not look as a good idea to change
the JVMTI phase like above.<o:p></o:p></p>
<p class="MsoNormal"> If you need the ONLOAD phase just to
enable capabilities then it is better to do it in the real
ONLOAD phase.<o:p></o:p></p>
<p class="MsoNormal"> Do I miss anything important here?<o:p></o:p></p>
<p class="MsoNormal"> Please, ask questions if you have any
problems with it."<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">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.<o:p></o:p></p>
<p class="MsoNormal">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).<o:p></o:p></p>
<p class="MsoNormal">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.<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">"The Jfr::on_vm_init() is confusing as
there is a mismatch with the JVMTI phases order.<o:p></o:p></p>
<p class="MsoNormal"> It fills like it means JFR init event
(not VM init) or something like this.<o:p></o:p></p>
<p class="MsoNormal"> Or maybe it denotes the VM initialization
start. :)<o:p></o:p></p>
<p class="MsoNormal"> I'll be happy if you could explain it a
little bit."<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">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 <span style="font-family:Wingdings">J</span>. 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...<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">I abandoned the attempt to encode anything
meaningful into the JFR calls trying to represent a certain
"VM initialization stage".<o:p></o:p></p>
<p class="MsoNormal">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. <o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">“Not sure, if your agent needs to enable
these capabilities (introduced in JDK 9 with modules):<br>
can_generate_early_vmstart<br>
can_generate_early_class_hook_events”<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">Thanks for the suggestion Serguei, but
these capabilities are not yet needed.<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">Here is the updated webrev: <a
href="http://cr.openjdk.java.net/~mgronlun/8233197/webrev02/"
moz-do-not-send="true">http://cr.openjdk.java.net/~mgronlun/8233197/webrev02/</a><o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">Thanks again<o:p></o:p></p>
<p class="MsoNormal">Markus<o:p></o:p></p>
<p class="MsoNormal"><span style="color:windowtext" lang="SV"><o:p> \
</o:p></span></p>
<p class="MsoNormal"><span style="color:windowtext" lang="SV"><o:p> \
</o:p></span></p> <div>
<div style="border:none;border-top:solid #E1E1E1
1.0pt;padding:3.0pt 0cm 0cm 0cm">
<p class="MsoNormal"><b><span \
style="color:windowtext">From:</span></b><span style="color:windowtext"> Serguei \
Spitsyn <br> <b>Sent:</b> den 20 november 2019 04:10<br>
<b>To:</b> Markus Gronlund
<a class="moz-txt-link-rfc2396E" \
href="mailto:markus.gronlund@oracle.com"><markus.gronlund@oracle.com></a>; \
hotspot-jfr-dev
<a class="moz-txt-link-rfc2396E" \
href="mailto:hotspot-jfr-dev@openjdk.java.net"><hotspot-jfr-dev@openjdk.java.net></a>;
<a class="moz-txt-link-abbreviated" \
href="mailto:hotspot-runtime-dev@openjdk.java.net">hotspot-runtime-dev@openjdk.java.net</a>;
<a class="moz-txt-link-abbreviated" \
href="mailto:serviceability-dev@openjdk.java.net">serviceability-dev@openjdk.java.net</a><br>
<b>Subject:</b> Re: 8233197(S): Invert
JvmtiExport::post_vm_initialized() and Jfr:on_vm_start()
start-up order for correct option parsing<o:p></o:p></span></p>
</div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<p class="MsoNormal">Hi Marcus,<br>
<br>
It looks good in general.<br>
<br>
A couple of comments though.<br>
<br>
<a
href="http://cr.openjdk.java.net/~mgronlun/8233197/webrev01/src/hotspot/share/jfr/instrumentation/jfrJvmtiAgent.cpp.frames.html"
moz-do-not-send="true">http://cr.openjdk.java.net/~mgronlun/8233197/webrev01/src/hotspot/share/jfr/instrumentation/jfrJvmtiAgent.cpp.frames.html</a><o:p></o:p></p>
<pre><span class="new"> 258 class JvmtiPhaseTransition \
{</span><o:p></o:p></pre> <pre><span class="new"> 259 \
private:</span><o:p></o:p></pre>
<pre><span class="new"> 260 bool _transition;</span><o:p></o:p></pre>
<pre><span class="new"> 261 public:</span><o:p></o:p></pre>
<pre><span class="new"> 262 JvmtiPhaseTransition() : \
_transition(JvmtiEnvBase::get_phase() == JVMTI_PHASE_PRIMORDIAL) \
{</span><o:p></o:p></pre>
<pre><span class="new"> 263 if (_transition) {</span><o:p></o:p></pre>
<pre><span class="new"> 264 \
JvmtiEnvBase::set_phase(JVMTI_PHASE_ONLOAD);</span><o:p></o:p></pre> <pre><span \
class="new"> 265 }</span><o:p></o:p></pre> <pre><span class="new"> 266 \
}</span><o:p></o:p></pre>
<pre><span class="new"> 267 ~JvmtiPhaseTransition() \
{</span><o:p></o:p></pre>
<pre><span class="new"> 268 if (_transition) {</span><o:p></o:p></pre>
<pre><span class="new"> 269 assert(JvmtiEnvBase::get_phase() == \
JVMTI_PHASE_ONLOAD, "invariant");</span><o:p></o:p></pre>
<pre><span class="new"> 270 \
JvmtiEnvBase::set_phase(JVMTI_PHASE_PRIMORDIAL);</span><o:p></o:p></pre> <pre><span \
class="new"> 271 }</span><o:p></o:p></pre> <pre><span class="new"> 272 \
}</span><o:p></o:p></pre> <pre><span class="new"> 273 };</span><o:p></o:p></pre>
<pre><span class="new"> 274 </span><o:p></o:p></pre>
<pre> 275 static bool initialize() {<o:p></o:p></pre>
<pre> 276 JavaThread* const jt = current_java_thread();<o:p></o:p></pre>
<pre> 277 assert(jt != NULL, "invariant");<o:p></o:p></pre>
<pre> 278 assert(jt->thread_state() == _thread_in_vm, \
"invariant");<o:p></o:p></pre>
<pre> 279 \
DEBUG_ONLY(JfrJavaSupport::check_java_thread_in_vm(jt));<o:p></o:p></pre>
<pre><span class="new"><b> 280 JvmtiPhaseTransition \
jvmti_phase_transition;</b></span><o:p></o:p></pre>
<pre> 281 ThreadToNativeFromVM transition(jt);<o:p></o:p></pre>
<pre> 282 if (create_jvmti_env(jt) != JNI_OK) {<o:p></o:p></pre>
<pre> 283 assert(jfr_jvmti_env == NULL, "invariant");<o:p></o:p></pre>
<pre> 284 return false;<o:p></o:p></pre>
<pre> 285 }<o:p></o:p></pre>
<pre> 286 assert(jfr_jvmti_env != NULL, "invariant");<o:p></o:p></pre>
<pre><span class="changed"> 287 if (!register_capabilities(jt)) \
{</span><o:p></o:p></pre> <pre> 288 return false;<o:p></o:p></pre>
<pre> 289 }<o:p></o:p></pre>
<pre><span class="changed"> 290 if (!register_callbacks(jt)) \
{</span><o:p></o:p></pre> <pre> 291 return false;<o:p></o:p></pre>
<pre> 292 }<o:p></o:p></pre>
<pre><span class="changed"> 293 return \
update_class_file_load_hook_event(JVMTI_ENABLE);</span><o:p></o:p></pre> <pre> 294 \
}<o:p></o:p></pre> <p class="MsoNormal"><br>
It does not look as a good idea to change the JVMTI phase
like above.<br>
If you need the ONLOAD phase just to enable capabilities
then it is better to do it in the real ONLOAD phase.<br>
Do I miss anything important here?<br>
Please, ask questions if you have any problems with it.<br>
<br>
The Jfr::on_vm_init() is confusing as there is a mismatch
with the JVMTI phases order.<br>
It fills like it means JFR init event (not VM init) or
something like this.<br>
Or maybe it denotes the VM initialization start. :)<br>
I'll be happy if you could explain it a little bit.<br>
<br>
Not sure, if your agent needs to enable these capabilities
(introduced in JDK 9 with modules):<br>
can_generate_early_vmstart<br>
can_generate_early_class_hook_events<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
On 11/19/19 06:38, Markus Gronlund wrote:<o:p></o:p></p>
</div>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<pre>Greetings,<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>(apologies for the wide distribution)<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>Kindly asking for reviews for the following \
changeset:<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre>Bug: <a href="https://bugs.openjdk.java.net/browse/JDK-8233197" \
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8233197</a> \
<o:p></o:p></pre> <pre>Webrev: <a \
href="http://cr.openjdk.java.net/~mgronlun/8233197/webrev01/" \
moz-do-not-send="true">http://cr.openjdk.java.net/~mgronlun/8233197/webrev01/</a><o:p></o:p></pre>
<pre>Testing: serviceability/jvmti, jdk_jfr, tier1-5<o:p></o:p></pre>
<pre>Summary: please see bug for description.<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>For Runtime / Serviceability folks:<o:p></o:p></pre>
<pre>This change slightly modifies the relative order in \
Threads::create_vm(); please see threads.cpp.<o:p></o:p></pre>
<pre>There is an upcall as part of Jfr::on_vm_start() that delivers global \
JFR command-line options to Java (only if set).<o:p></o:p></pre> <pre>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.<o:p></o:p></pre> \
<pre><o:p> </o:p></pre> <pre>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.<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre>Thank you<o:p></o:p></pre>
<pre>Markus<o:p></o:p></pre>
</blockquote>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
</blockquote>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic