[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFS(S): 8222934: mark new VM option AllowRedefinitionToAddOrDeleteMethods as deprecated
From: "serguei.spitsyn () oracle ! com" <serguei ! spitsyn () oracle ! com>
Date: 2019-04-26 18:40:17
Message-ID: d74da06b-5769-2b26-060e-eae666edfa93 () oracle ! com
[Download RAW message or body]
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">Thanks, Coleen!<br>
Serguei<br>
<br>
<br>
On 4/26/19 11:39, <a class="moz-txt-link-abbreviated" \
href="mailto:coleen.phillimore@oracle.com">coleen.phillimore@oracle.com</a> \
wrote:<br> </div>
<blockquote type="cite"
cite="mid:bedd84cd-7f78-70ed-efd2-878f654bb069@oracle.com"> Looks
good to me.<br>
Coleen<br>
<br>
<div class="moz-cite-prefix">On 4/26/19 2:31 PM, <a
class="moz-txt-link-abbreviated"
href="mailto:serguei.spitsyn@oracle.com"
moz-do-not-send="true">serguei.spitsyn@oracle.com</a> wrote:<br>
</div>
<blockquote type="cite"
cite="mid:24650188-189e-c98e-db86-9ecc31e94e52@oracle.com">
<div class="moz-cite-prefix">Hi Dan,<br>
<br>
Thank you for re-review!<br>
I'll fix the spaces at the end, thanks.<br>
<br>
Thanks,<br>
Serguei<br>
<br>
On 4/26/19 08:27, Daniel D. Daugherty wrote:<br>
</div>
<blockquote type="cite"
cite="mid:271c69c9-6a38-50b5-f567-688f164ad4a9@oracle.com"> On
4/25/19 10:57 PM, <a class="moz-txt-link-abbreviated"
href="mailto:serguei.spitsyn@oracle.com"
moz-do-not-send="true">serguei.spitsyn@oracle.com</a> wrote:<br>
<blockquote type="cite"
cite="mid:d4ce83fd-74ab-604e-85c9-4b3f1c2bf5fc@oracle.com">
Hi Coleen and Dan,<br>
<br>
Updated webrev is:<br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/~sspitsyn/webrevs/2019/8222934-jvmti-depr-option.2/"
moz-do-not-send="true">http://cr.openjdk.java.net/~sspitsyn/webrevs/2019/8222934-jvmti-depr-option.2/</a><br>
</blockquote>
<tt> <br>
src/hotspot/share/runtime/globals.hpp<br>
No comments.<br>
<br>
test/hotspot/jtreg/runtime/CommandLine/VMDeprecatedOptions.java<br>
No comments.<br>
<br>
test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/TestAddDeleteMethods.java<br>
L100: // So, this redefinition will add it back
which is expected to work.<br>
There's a space at the end of this comment line.
jcheck may complain.<br>
<br>
L132: // So, this redefinition will deleate it back
which is expected to work.<br>
Typo: s/deleate it back/delete it/<br>
<br>
There's a space at the end of this comment line.
jcheck may complain.<br>
<br>
Thumbs up.<br>
<br>
Dan<br>
<br>
<br>
<br>
</tt>
<blockquote type="cite"
cite="mid:d4ce83fd-74ab-604e-85c9-4b3f1c2bf5fc@oracle.com">
<br>
This implements the suggestions:<br>
VMDeprecatedOptions.java:<br>
- moved the option to the deprecated non-alias flags
section<br>
<br>
TestAddDeleteMethods.java:<br>
- removed confusion in redefinition string names and added
comments recommended by Dan<br>
- always list methods in order: foo, publicFoo, finalFoo,
staticFoo<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
<div class="moz-cite-prefix">On 4/25/19 2:41 PM, <a
class="moz-txt-link-abbreviated"
href="mailto:serguei.spitsyn@oracle.com"
moz-do-not-send="true">serguei.spitsyn@oracle.com</a>
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:f1117762-4f93-cb03-53f4-d06b78e51f33@oracle.com">
Hi Coleen,<br>
<br>
Thank you a lot for looking at this!<br>
<br>
<br>
<div class="moz-cite-prefix">On 4/25/19 2:18 PM, <a
class="moz-txt-link-abbreviated"
href="mailto:coleen.phillimore@oracle.com"
moz-do-not-send="true">coleen.phillimore@oracle.com</a>
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:2531112b-7ec4-1190-0fc5-f88af38b418b@oracle.com">
<br>
<br>
<div class="moz-cite-prefix">On 4/25/19 4:19 PM, <a
class="moz-txt-link-abbreviated"
href="mailto:serguei.spitsyn@oracle.com"
moz-do-not-send="true">serguei.spitsyn@oracle.com</a>
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:cc740917-c713-155c-aba7-3fa60dcc6988@oracle.com">
<div class="moz-cite-prefix">Hi Dan,<br>
<br>
Thank you a lot fore reviewing this!<br>
<br>
On 4/25/19 12:40, Daniel D. Daugherty wrote:<br>
</div>
<blockquote type="cite"
cite="mid:9de5a830-6f6e-ba37-8bc0-81f8ffce3c48@oracle.com">On
4/24/19 6:18 PM, <a
class="moz-txt-link-abbreviated"
href="mailto:serguei.spitsyn@oracle.com"
moz-do-not-send="true">serguei.spitsyn@oracle.com</a>
wrote: <br>
<blockquote type="cite">Please, review fix for: <br>
<a class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8222934"
\
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8222934</a> <br>
<br>
Webrev: <br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Esspitsyn/webrevs/2019/8222934-jvmti-depr-option.1/"
\
moz-do-not-send="true">http://cr.openjdk.java.net/~sspitsyn/webrevs/2019/8222934-jvmti-depr-option.1/</a>
<br>
</blockquote>
<br>
src/hotspot/share/runtime/globals.hpp <br>
No comments. <br>
<br>
test/hotspot/jtreg/runtime/CommandLine/VMDeprecatedOptions.java <br>
L42: // deprecated class redefinition
flags: <br>
L43:
{"AllowRedefinitionToAddDeleteMethods", "true"}, <br>
L44: <br>
L45: // deprecated non-alias flags: <br>
I think your new flag entry should have been
added to the <br>
"deprecated non-alias flags" section. You
don't need to <br>
call out that this is a "class redefinition"
flag. <br>
<br>
The reason for the "// deprecated alias
flags (see also aliased_jvm_flags):" <br>
section (below what you changed) is because
there is more <br>
work to do for those flags.<br>
</blockquote>
<br>
Okay, I'm not very familiar with this test, will check
how to change it.<br>
<br>
<blockquote type="cite"
cite="mid:9de5a830-6f6e-ba37-8bc0-81f8ffce3c48@oracle.com">
<br>
test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/TestAddDeleteMethods.java
<br>
L94: public static String ADeleteStaticFoo =
<br>
This case is deleting both "staticFoo" and
"finalFoo". <br>
Is that what you really want? If so, then
the test case <br>
is misnamed.<br>
</blockquote>
<br>
I see your confusion here.<br>
The ADeleteStaticFoo is used after the <span
class="changed">ADeleteFinalFoo.<br>
So, the </span>"finalFoo" has been already deleted
before.<br>
Then the ADeleteStaticFoo only deletes the
"staticFoo".<br>
<br>
The same was not the case for <span \
class="changed">ADeleteFinalFoo.<br> It is because the redefinitions with \
</span><span class="changed">ADeleteFoo and </span><span
class="changed">ADeletePublicFoo<br>
are expected to be rejected with UOE.<br>
</span><span class="changed"></span><br>
<span class="changed"></span>
<blockquote type="cite"
cite="mid:9de5a830-6f6e-ba37-8bc0-81f8ffce3c48@oracle.com">
<br>
L119 public static String BAddStaticBar = <br>
This case is added "staticBar" and
"finalBar". Is that what <br>
you really want? If so, then the test case
is misnamed.<br>
</blockquote>
<br>
This one is similar to the above.<br>
The "finalBar" has already been added by<span
class="changed"> the BAddFinalBar </span>redefinition.<br>
<br>
Please, let me know if you are Okay with it as it is
or prefer to add a comment with clarification.<br>
<br>
<blockquote type="cite"
cite="mid:9de5a830-6f6e-ba37-8bc0-81f8ffce3c48@oracle.com">
<br>
Still a really cool test here! <br>
</blockquote>
<br>
The test was initially written by Coleen (thanks,
Coleen!)<br>
I've spoiled it a little bit though. :)<br>
</blockquote>
<br>
Hi Serguei, You added a lot to it, which is taking me a
while to understand.<br>
<br>
Why did you make class A inherit from Runnable?<br>
</blockquote>
<br>
In fact, nothing foxy.<br>
It implements Runnable, not inherits. :)<br>
There were two steps.<br>
First was to decide if we there is a point to call methods
in the redefined classes A and B.<br>
You did it with the in the original test version but you
made publicFoo to call others.<br>
So, I decided that it is useful to make sure the methods
are executed well after redefinition.<br>
Then I decided to use another class B for added methods.<br>
Calling other methods from publicFoo did not work for me.<br>
I had to generalize it with run() method and then made<br>
classes A and B to implement Runnable to make it more
clear.<br>
<br>
<br>
<blockquote type="cite"
cite="mid:2531112b-7ec4-1190-0fc5-f88af38b418b@oracle.com">
Can you maintain order of the function declarations?<br>
<br>
<pre> 78 public static String ADeletePublicFoo =
</pre>
<br>
finalFoo should be before staticFoo in this one.<br>
</blockquote>
Nice catch, thanks!<br>
Will fix it in the webrev update.<br>
<br>
<blockquote type="cite"
cite="mid:2531112b-7ec4-1190-0fc5-f88af38b418b@oracle.com">
<br>
Oh, now I see what Dan is talking about. In
ADeleteStaticFoo, finalFoo has already been deleted so
you didn't want to also test adding it back.<br>
</blockquote>
<br>
Right.<br>
<br>
<blockquote type="cite"
cite="mid:2531112b-7ec4-1190-0fc5-f88af38b418b@oracle.com">
<br>
Thank you for enhancing the test. I guess it's good
that it tests the new option.<br>
</blockquote>
<br>
Thanks!<br>
Serguei<br>
<br>
<blockquote type="cite"
cite="mid:2531112b-7ec4-1190-0fc5-f88af38b418b@oracle.com">
<br>
Coleen<br>
<br class="Apple-interchange-newline">
<blockquote type="cite"
cite="mid:cc740917-c713-155c-aba7-3fa60dcc6988@oracle.com">
<br>
<blockquote type="cite"
cite="mid:9de5a830-6f6e-ba37-8bc0-81f8ffce3c48@oracle.com">
<br>
Thumbs up. Your call on whether to tweak the test. <br>
</blockquote>
<br>
I'll send a VMDeprecatedOptions related update later.<br>
<br>
<br>
Thanks!<br>
Serguei<br>
<blockquote type="cite"
cite="mid:9de5a830-6f6e-ba37-8bc0-81f8ffce3c48@oracle.com">
<br>
Dan <br>
<br>
<br>
<blockquote type="cite"> <br>
<br>
Summary: <br>
David, in review for <a
class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8222934"
\
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8222934</a> \
suggested: <br> 1. I would have suggested to add "(Deprecated)"
to the description of the new flag in globals.hpp
<br>
2. The new flag should have been added to the
deprecated VM options tests. <br>
3. The new test should run in both a positive
and negative mode so that it also checks that the
new flag works. <br>
<br>
The webrev above implements this suggestion. <br>
<br>
<br>
Testing: <br>
In progress: Submit mach5 run for the updated
tests. <br>
<br>
<br>
Thanks, <br>
Serguei <br>
<br>
<br>
<br>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic