Re: RFR: 8240698: LingeredApp does not pass getTestJavaOpts() to the children process if vmArguments is already specified
Hi Leonid,
On 3/30/20 5:42 PM, Leonid Mesnik wrote:
Hi
See my comments inline. I will update webrev after go through all your
comments.
On 3/30/20 11:39 AM, Chris Plummer wrote:
Hi Leonid,
I haven't gone through all the tests yet. I've accumulated enough
questions that I'd like to see them answered or addressed before I
continue on.
This isn't directly related to your changes, but I noticed that users
of JDKToolLauncher do nothing to make sure that default test options
are used. This means we are never running these tools with the test
options being specified with the jtreg run. Is that a bug or
intentional?
Which "default test options" do you mean? We have 2 properties to set
JVM options. The idea is to pass test.vm.opts to ALL java processes
and test.java.opts to only tested processes if applicable. Usually,
for example we don't want to run jcmd with -Xcomp. test.vm.opts was
used (a long time ago) for options like '-d32/-d64' on Solaris where
JVM don't start without choosing correct version. Also, it is used to
reduce maximum heap for all JVM instances when tests are running
concurrently.
So, probably test.vm.opts (or test.vm.tools.opts) should be added by
JDKToolLauncher but not test.java.opts. It is separate topic, there
are a lot of launchers which ignore test.vm.opts now.
I always get confused about which set of options these properties
represent, but basically I'm suggesting that if for example we are doing
a -Xcomp run in mach5, JDKToolLauncher (at least in some cases) should
be launched with this option. I think this is what you get from
Utils.getTestJavaOpts(),.
For example the SA tests use
JDKToolLauncher.createUsingTestJDK("jhsdb"). jhsdb is what is really
being tested here, and it should be launched with the test vm options.
Currently we launch the target process with these options, which is
probably also a good idea. Also we aren't too concerned with the
options that the test itself is run with, although I'm guessing they
also get run with the test java opts. So we have 3 processes here:
- jhsdb, which should be getting test java opts but is not
- the target process, which should be getting test java opts and
currently is
- the test itself, where options don't really matter, but is getting
passed test java opts
However, you could argue that tests like jinfo, jstack, and jcmd, all of
which use the Attach API and the bulk of the work is done on the target
process, are not that concerned with the options passed to the command,
but do want the options passed to the target process.
In the problem lists, is it necessary to list the test multiple times
with #id0, #id1, etc, or could you list it just once and leave that
part off. It seems very error prone. Also, changing tests like
ClhsdbFindPC, ClhsdbJstack, and ClhsdbScanOops to split out the
testing in this manner seems completely unrelated to this CR,
especially when the tests do not even contain any changes related to
the CR.
I think, that these chages are related. The startApp(...) was updated
so some test combinations become invalid or redundant.
ClhsdbFindPC and ClhsdbJstack were always run twice. Now, when test
options passed in test it is not needed to run it twice when Xcomp is
already set by user.
Ok. I see now that the second test run, which is the non -Xcomp run,
adds '@requires vm.compMode != "Xcomp"'. But this also is strange. The
first test run, which does not have the @requires and is the one that
makes LingeredApp launch with -Xcomp, will always run whether or not it
is an -Xcomp test run. So it will run as part of the a regular test run
and as part of a -Xcomp test run. The only difference between the two is
the -Xcomp run will also run the test with -Xcomp, but that's not really
needed (I think it will also end up passing -Xcomp to the target
processs twice). Perhaps '@requires vm.compMode == "Xcomp"' should be
used for the first test run, but that means it no longer gets run until
later tiers when we use -Xcomp. Why not revert it back to a single test,
but also add '@requires vm.compMode != "Xcomp"'. Then it gets run both
ways in an early tier and not run during the -Xcomp run, which isn't
really needed.
ClhsdbScanOops is fixed to don't allow to run incompatible GC
combination.
Ok
So I should update these tests by splitting them or change them to
startAppExactJvmOpts() if we wan't continue to ignore user-given test
options.
I don't think I was suggesting removing user-given test options. I don't
see why you would.
It seems that #idN are required by jtreg now, otherwise it just run test.
Ok.
426 public static LingeredApp startApp(String...
additionalJvmOpts) throws IOException {
The default test opts are appended to additionalJvmOpts, and if you
want prepended you need to call Utils.prependTestJavaOpts(). I would
have thought the opposite would be more desirable and expected
default behavior. Why did you choose thi
Re: RFR: 8240698: LingeredApp does not pass getTestJavaOpts() to the children process if vmArguments is already specified
Hi
See my comments inline. I will update webrev after go through all your
comments.
On 3/30/20 11:39 AM, Chris Plummer wrote:
Hi Leonid,
I haven't gone through all the tests yet. I've accumulated enough
questions that I'd like to see them answered or addressed before I
continue on.
This isn't directly related to your changes, but I noticed that users
of JDKToolLauncher do nothing to make sure that default test options
are used. This means we are never running these tools with the test
options being specified with the jtreg run. Is that a bug or intentional?
Which "default test options" do you mean? We have 2 properties to set
JVM options. The idea is to pass test.vm.opts to ALL java processes and
test.java.opts to only tested processes if applicable. Usually, for
example we don't want to run jcmd with -Xcomp. test.vm.opts was used (a
long time ago) for options like '-d32/-d64' on Solaris where JVM don't
start without choosing correct version. Also, it is used to reduce
maximum heap for all JVM instances when tests are running concurrently.
So, probably test.vm.opts (or test.vm.tools.opts) should be added by
JDKToolLauncher but not test.java.opts. It is separate topic, there are
a lot of launchers which ignore test.vm.opts now.
In the problem lists, is it necessary to list the test multiple times
with #id0, #id1, etc, or could you list it just once and leave that
part off. It seems very error prone. Also, changing tests like
ClhsdbFindPC, ClhsdbJstack, and ClhsdbScanOops to split out the
testing in this manner seems completely unrelated to this CR,
especially when the tests do not even contain any changes related to
the CR.
I think, that these chages are related. The startApp(...) was updated so
some test combinations become invalid or redundant.
ClhsdbFindPC and ClhsdbJstack were always run twice. Now, when test
options passed in test it is not needed to run it twice when Xcomp is
already set by user.
ClhsdbScanOops is fixed to don't allow to run incompatible GC combination.
So I should update these tests by splitting them or change them to
startAppExactJvmOpts() if we wan't continue to ignore user-given test
options.
It seems that #idN are required by jtreg now, otherwise it just run test.
426 public static LingeredApp startApp(String...
additionalJvmOpts) throws IOException {
The default test opts are appended to additionalJvmOpts, and if you
want prepended you need to call Utils.prependTestJavaOpts(). I would
have thought the opposite would be more desirable and expected default
behavior. Why did you choose this way? I also find it somewhat
confusing that there is even a default mode for where the
additionalJvmOpts go. Maybe it would be best to have
startAppAppendJvmArgs() and startAppPrependJvmArgs() just to make it
explicit. This would also be in line with the existing
startAppExactJvmOpts().
I've chosen the most popular usage, which was Utils.appendTestJavaOpts.
But I agree, that it would be better to change it to prepend. Thanks for
pointing to this.
I don't want to add startAppAppendJvmArgs()/startAppPrependJvmArgs() to
don't complicate all things. I think that startApp() should be used in
the cases when test vm options really shouldn't interfere with
user-provided options or overwrite them. So basically the behavior is
the same as for ProcessTools.createJavaProcessBuilder(true, ...) and
jtreg itself.
Is ClhsdbFindPC correct. It used to use just use -Xcomp or -Xint,
ignoring any default test opts. You've fixed it to include the default
test opts, but the are appended, possibly overriding the -Xcomp or
-Xint. Don't we want the default test opts prepended? Same for
ClhsdbJstack.
The idea is to don't mix Xcomp and Xmixed/Xint using requires filter.
However ClhsdbFindPC might override Xint with Xmixed if it is set
explicitly. Switching to prepending will fix it.
Leonid
thanks,
Chris
On 3/25/20 2:31 PM, Leonid Mesnik wrote:
Igor, Stefan, Ioi
Thank you for your feedback.
Filed https://bugs.openjdk.java.net/browse/JDK-8241624 To change @run
main... to @run driver.
Test ClhsdbJstack.java is updated.
Still waiting for review from SVC team.
webrev: http://cr.openjdk.java.net/~lmesnik/8240698/webrev.02/
Leonid
On 3/25/20 12:46 PM, Igor Ignatyev wrote:
Hi Leonid,
not related related to your patch (but yet somewhat made more
obvious by it), it seems all (or at least almost all) the tests
which use�LingeredApp should be run in "driver" mode as they just
orchestrate execution of other JVMs, so running them w/ main (let
alone main/othervm) just wastes time,
test/hotspot/jtreg/serviceability/sa/ClhsdbJstack.java#id1, for
example, will now executed w/ Xcomp which will make it very slow for
no reasons. since you already got your hands dirty w/ these tests,
could you please file an RFE to sort this out and list all the
affected tests there?
re: the patch, could you please update ClhsdbJstack.ja
Re: RFR: 8241530: com/sun/jdi tests fail due to network issues on OSX 10.15
Looks good. --alex On 03/30/2020 12:43, Daniil Titov wrote: Please review the change [1] that fixes the failure of com/sun/jdi/JdwpListenTest.java and com/sun/jdi/JdwpAttachTest.java tests on OSX 10.15. The problem here is the similar to the one solved in [4] by additional filtering of unusual network interfaces in the test library class jdk.test.lib.NetworkConfiguration. However, the failing com/sun/jdi tests do not use jdk.test.lib.NetworkConfiguration and Instead do repeat the same logic themselves. The fix changes these tests to start using jdk.test.lib.NetworkConfiguration to find all local addresses. Initially the issue [2] also included 3 other failing tests from sun/management/jdp package, but these tests fail for a different reason so I moved them in the new issue [3] and updated the ProblemList.txt for them. [1] Webrev: http://cr.openjdk.java.net/~dtitov/8241530/webrev.01/ [2] Jira Issue: https://bugs.openjdk.java.net/browse/JDK-8241530 [3] https://bugs.openjdk.java.net/browse/JDK-8241865 [4] https://bugs.openjdk.java.net/browse/JDK-8241336 Thank you, Daniil
RFR: 8241530: com/sun/jdi tests fail due to network issues on OSX 10.15
Please review the change [1] that fixes the failure of com/sun/jdi/JdwpListenTest.java and com/sun/jdi/JdwpAttachTest.java tests on OSX 10.15. The problem here is the similar to the one solved in [4] by additional filtering of unusual network interfaces in the test library class jdk.test.lib.NetworkConfiguration. However, the failing com/sun/jdi tests do not use jdk.test.lib.NetworkConfiguration and Instead do repeat the same logic themselves. The fix changes these tests to start using jdk.test.lib.NetworkConfiguration to find all local addresses. Initially the issue [2] also included 3 other failing tests from sun/management/jdp package, but these tests fail for a different reason so I moved them in the new issue [3] and updated the ProblemList.txt for them. [1] Webrev: http://cr.openjdk.java.net/~dtitov/8241530/webrev.01/ [2] Jira Issue: https://bugs.openjdk.java.net/browse/JDK-8241530 [3] https://bugs.openjdk.java.net/browse/JDK-8241865 [4] https://bugs.openjdk.java.net/browse/JDK-8241336 Thank you, Daniil
Re: Review Request: 8238358: Implementation of JEP 371: Hidden Classes
This is the patch to keep the JDK 14 behavior if target release to 14 (thanks to Jan for helping making change in javac to get the tests working) http://cr.openjdk.java.net/~mchung/valhalla/webrevs/8171335/webrev-javac-target-release-14/ Mandy On 3/27/20 9:29 AM, Mandy Chung wrote: Hi Jan, Good point. The javac change only applies to JDK 15 and later and the lambda proxy class is not a nestmate when running on JDK 14 or earlier. I probably need the help from langtools team to fix this. I'll give it a try. Mandy
Re: Discussion about fixing deprecation in jdk.hotspot.agent
I was wondering why this is needed when debugging a core file, which is
the key thing we need the SA for:
/** This is used by both the debugger and any runtime system. It is
the basic mechanism by which classes which mimic underlying VM
functionality cause themselves to be initialized. The given
observer will be notified (with arguments (null, null)) when the
VM is re-initialized, as well as when it registers itself with
the VM. */
public static void registerVMInitializedObserver(Observer o) {
vmInitializedObservers.add(o);
o.update(null, null);
}
It seems like if it isn't needed, we shouldn't add these classes and
remove their use.
Coleen
On 3/30/20 8:14 AM, Magnus Ihse Bursie wrote:
No opinions on this?
/Magnus
On 2020-03-25 23:34, Magnus Ihse Bursie wrote:
Hi everyone,
As a follow-up to the ongoing review for JDK-8241618, I have also
looked at fixing the deprecation warnings in jdk.hotspot.agent. These
fall in three broad categories:
* Deprecation of the boxing type constructors (e.g. "new Integer(42)").
* Deprecation of java.util.Observer and Observable.
* The rest (mostly Class.newInstance(), and a few number of other odd
deprecations)
The first category is trivial to fix. The last category need some
special discussion. But the overwhelming majority of deprecation
warnings come from the use of Observer and Observable. This really
dwarfs anything else, and needs to be handled first, otherwise it's
hard to even spot the other issues.
My analysis of the situation is that the deprecation of Observer and
Observable seems a bit harsh, from the PoV of jdk.hotspot.agent.
Sure, it might be limited, but I think it does exactly what is needed
here. So the migration suggested in Observable (java.beans or
java.util.concurrent) seems overkill. If there are genuine threading
issues at play here, this assumption might be wrong, and then maybe
going the j.u.c. route is correct.
But if that's not, the main goal should be to stay with the current
implementation. One way to do this is to sprinkle the code with
@SuppressWarning. But I think a better way would be to just implement
our own Observer and Observable. After all, the classes are trivial.
I've made a mock-up of this solution, were I just copied the
java.util.Observer and Observable, and removed the deprecation
annotations. The only thing needed for the rest of the code is to
make sure we import these; I've done this for three arbitrarily
selected classes just to show what the change would typically look
like. Here's the mock-up:
http://cr.openjdk.java.net/~ihse/hotspot-agent-observer/webrev.01
Let me know what you think.
/Magnus
Re: RFR: 8240698: LingeredApp does not pass getTestJavaOpts() to the children process if vmArguments is already specified
Hi Leonid,
I haven't gone through all the tests yet. I've accumulated enough
questions that I'd like to see them answered or addressed before I
continue on.
This isn't directly related to your changes, but I noticed that users of
JDKToolLauncher do nothing to make sure that default test options are
used. This means we are never running these tools with the test options
being specified with the jtreg run. Is that a bug or intentional?
In the problem lists, is it necessary to list the test multiple times
with #id0, #id1, etc, or could you list it just once and leave that part
off. It seems very error prone. Also, changing tests like ClhsdbFindPC,
ClhsdbJstack, and ClhsdbScanOops to split out the testing in this manner
seems completely unrelated to this CR, especially when the tests do not
even contain any changes related to the CR.
426 public static LingeredApp startApp(String...
additionalJvmOpts) throws IOException {
The default test opts are appended to additionalJvmOpts, and if you want
prepended you need to call Utils.prependTestJavaOpts(). I would have
thought the opposite would be more desirable and expected default
behavior. Why did you choose this way? I also find it somewhat confusing
that there is even a default mode for where the additionalJvmOpts go.
Maybe it would be best to have startAppAppendJvmArgs() and
startAppPrependJvmArgs() just to make it explicit. This would also be in
line with the existing startAppExactJvmOpts().
Is ClhsdbFindPC correct. It used to use just use -Xcomp or -Xint,
ignoring any default test opts. You've fixed it to include the default
test opts, but the are appended, possibly overriding the -Xcomp or
-Xint. Don't we want the default test opts prepended? Same for ClhsdbJstack.
thanks,
Chris
On 3/25/20 2:31 PM, Leonid Mesnik wrote:
Igor, Stefan, Ioi
Thank you for your feedback.
Filed https://bugs.openjdk.java.net/browse/JDK-8241624 To change @run
main... to @run driver.
Test ClhsdbJstack.java is updated.
Still waiting for review from SVC team.
webrev: http://cr.openjdk.java.net/~lmesnik/8240698/webrev.02/
Leonid
On 3/25/20 12:46 PM, Igor Ignatyev wrote:
Hi Leonid,
not related related to your patch (but yet somewhat made more obvious
by it), it seems all (or at least almost all) the tests which
use�LingeredApp should be run in "driver" mode as they just
orchestrate execution of other JVMs, so running them w/ main (let
alone main/othervm) just wastes time,
test/hotspot/jtreg/serviceability/sa/ClhsdbJstack.java#id1, for
example, will now executed w/ Xcomp which will make it very slow for
no reasons. since you already got your hands dirty w/ these tests,
could you please file an RFE to sort this out and list all the
affected tests there?
re: the patch, could you please update ClhsdbJstack.java test not to
be run w/ Xcomp and follow the same pattern you used in other tests
(e.g.�ClhsdbScanOops) ? other than that it looks fine to me, I
however wouldn't be able to tell if all svc tests continue to do that
they were supposed to, so I'd prefer for someone from svc team
to�chime in.
Thanks,
-- Igor
On Mar 25, 2020, at 12:01 PM, Leonid Mesnik
mailto:leonid.mes...@oracle.com>> wrote:
Added Ioi, who also proposed new version of startAppVmOpts.
Please find new webrev:
http://cr.openjdk.java.net/~lmesnik/8240698/webrev.01/
Renamed startAppVmOpts/runAppVmOpts to
"startAppExactJvmOpts/runAppExactJvmOpts" is used. It should make
very clear that this method doesn't use any of test.java.opts,
test.vm.opts.
Also, I fixed test/hotspot/jtreg/serviceability/sa/ClhsdbFlags.java
metnioned by Igor, and removed null pointer check as Ioi suggested
in startApp method.
+ public static void startApp(LingeredApp theApp, String...
additionalJvmOpts) throws IOException {
+ startAppExactJvmOpts(theApp,
Utils.appendTestJavaOpts(additionalJvmOpts));
+ }
Leonid
On 3/25/20 10:14 AM, Stefan Karlsson wrote:
On 2020-03-25 17:40, Igor Ignatyev wrote:
Hi Leonid,
I have briefly looked at the patch, a few comments so far:
test/hotspot/jtreg/serviceability/sa/ClhsdbFlags.java:
� - at L#114, could you please call static method using class name
(as the opposite of using instance)? or was it meant to be
theApp.runAppVmOpts(vmArgs) ?
test/lib/jdk/test/lib/apps/LingeredApp.java:
- it seems that code indent of startApp(LingeredApp, String[])
isn't correct
- I don't like startAppVmOpts name, but unfortunately don't have a
better suggestion (yet)
I was going to say the same. Jtreg has the concept of "java
options" and "vm options". We have had a fair share of bugs and
wasted time when tests have been using the "vm options" part
(VM_OPTIONS, test.vm.options, etc), and we've been moving away from
using that way to pass options. I recently cleaned up some of this
with:
8237111: LingeredApp should be started with getTestJavaOpts
Because of this, I would prefer if we used a name that doesn't
include "VmOpts", because
Re: Review Request: 8238358: Implementation of JEP 371: Hidden Classes
On 3/30/20 02:30, serguei.spit...@oracle.com wrote:
Hi Mandy,
I have just one comment so far.
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03/src/hotspot/share/classfile/classLoaderHierarchyDCmd.cpp.frames.html
356 void add_classes(LoadedClassInfo* first_class, int num_classes,
bool has_class_mirror_holder) {
357 LoadedClassInfo** p_list_to_add_to;
358 bool is_hidden = first_class->_klass->is_hidden();
359 if (has_class_mirror_holder) {
360 p_list_to_add_to = is_hidden ? &_hidden_weak_classes :
&_anon_classes;
361 } else {
362 p_list_to_add_to = &_classes;
363 }
364 // Search tail.
365 while ((*p_list_to_add_to) != NULL) {
366 p_list_to_add_to = &(*p_list_to_add_to)->_next;
367 }
368 *p_list_to_add_to = first_class;
369 if (has_class_mirror_holder) {
370 if (is_hidden) {
371 _num_hidden_weak_classes += num_classes;
372 } else {
373 _num_anon_classes += num_classes;
374 }
375 } else {
376 _num_classes += num_classes;
377 }
378 }
Q1: I'm just curious, what happens if a cld has arrays of hidden
classes?
Is the bottom_klass always expected to be the first?
Please, skip it. I've got the answer.
The array classes were not included into the LoadedClassInfo* by the
classes_do.
Thanks,
Serguei
Thanks,
Serguei
On 3/26/20 16:57, Mandy Chung wrote:
Please review the implementation of JEP 371: Hidden Classes. The main
changes are in core-libs and hotspot runtime area. Small changes are
made in javac, VM compiler (intrinsification of
Class::isHiddenClass), JFR, JDI, and jcmd. CSR [1]has been reviewed
and is in the finalized state (see specdiff and javadoc below for
reference).
Webrev:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03
Hidden class is created via `Lookup::defineHiddenClass`. From JVM's
point
of view, a hidden class is a normal class except the following:
- A hidden class has no initiating class loader and is not registered
in any dictionary.
- A hidden class has a name containing an illegal character
`Class::getName` returns `p.Foo/0x1234` whereas `GetClassSignature`
returns "Lp/Foo.0x1234;".
- A hidden class is not modifiable, i.e. cannot be redefined or
retransformed. JVM TI IsModifableClass returns false on a hidden.
- Final fields in a hidden class is "final". The value of final
fields cannot be overriden via reflection. setAccessible(true) can
still be called on reflected objects representing final fields in a
hidden class and its access check will be suppressed but only have
read-access (i.e. can do Field::getXXX but not setXXX).
Brief summary of this patch:
1. A new Lookup::defineHiddenClass method is the API to create a
hidden class.
2. A new Lookup.ClassOption enum class defines NESTMATE and STRONG
option that
can be specified when creating a hidden class.
3. A new Class::isHiddenClass method tests if a class is a hidden class.
4. Field::setXXX method will throw IAE on a final field of a hidden
class
regardless of the value of the accessible flag.
5. JVM_LookupDefineClass is the new JVM entry point for
Lookup::defineClass
and defineHiddenClass to create a class from the given bytes.
6. ClassLoaderData implementation is not changed. There is one
primary CLD
that holds the classes strongly referenced by its defining
loader. There
can be zero or more additional CLDs - one per weak class.
7. Nest host determination is updated per revised JVMS 5.4.4. Access
control
check no longer throws LinkageError but instead it will throw IAE
with
a clear message if a class fails to resolve/validate the nest host
declared
in NestHost/NestMembers attribute.
8. JFR, jcmd, JDI are updated to support hidden classes.
9. update javac LambdaToMethod as lambda proxy starts using nestmates
and generate a bridge method to desuger a method reference to a
protected
method in its supertype in a different package
This patch also updates StringConcatFactory, LambdaMetaFactory, and
LambdaForms
to use hidden classes. The webrev includes changes in nashorn to
hidden class
and I will update the webrev if JEP 372 removes it any time soon.
We uncovered a bug in Lookup::defineClass spec throws LinkageError
and intends
to have the newly created class linked. However, the implementation
in 14
does not link the class. A separate CSR [2] proposes to update the
implementation to match the spec. This patch fixes the implementation.
The spec update on JVM TI, JDI and Instrumentation will be done as
a separate RFE [3]. This patch includes new tests for JVM TI and
java.instrument that validates how the existing APIs work for hidden
classes.
javadoc/specdiff
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/api/
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/specdiff/
JVMS 5.4.4 change:
http://cr.
Re: Review Request: 8238358: Implementation of JEP 371: Hidden Classes
On 3/30/20 7:16 AM, coleen.phillim...@oracle.com wrote: I agree with you that this comment needs update. Perhaps it should say "primitive, array types and hidden classes are non-modifiable. A modifiable class must be an InstanceKlass." I may have written the last part of that comment (or remember it at least). I think Chris's suggestion to remove the last sentence makes sense. Anything further will just adds unnecessary confusion to the reader. Anyone modifying this will get the InstanceKlass::cast() assert soon after if they mess up. OK. That's fine too. Mandy
Re: Review Request: 8238358: Implementation of JEP 371: Hidden Classes
Adding back hotspot-dev.
On 3/30/20 11:02 AM, coleen.phillim...@oracle.com wrote:
Hi, This is great work! I did a prereview and all of my comments
were addressed. These are a few minor things I noticed.
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03/src/hotspot/share/ci/ciInstanceKlass.hpp.udiff.html
Nit. Can you add 'const' to the is_hidden accessor?
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03/src/hotspot/share/classfile/classFileParser.cpp.udiff.html
+ ID annotation_index(const ClassLoaderData* loader_data, const
Symbol* name, const bool can_access_vm_annotations);
'const' bool is weird and unnecessary. Can you remove const here?
+ if (is_hidden()) { // Mark methods in hidden classes as 'hidden'.
+ m->set_hidden(true);
+ }
+
Could be:
+ // Mark methods in hidden classes as 'hidden'.
+ m->set_hidden(is_hidden());
+
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03/src/hotspot/share/classfile/javaClasses.cpp.udiff.html
+ macro(_classData_offset, k, "classData", object_signature, false); \
Probably should remove trailing backslash here.
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03/src/hotspot/share/classfile/systemDictionary.cpp.udiff.html
I think in a future RFE, we should add a default parameter to
register_loader to make the code in the beginning of parse_stream()
cleaner and remove has_class_mirror_holder_cld().
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03/src/hotspot/share/prims/jvm.cpp.udiff.html
+ jboolean is_nestmate = (flags & NESTMATE) == NESTMATE;
+ jboolean is_hidden = (flags & HIDDEN_CLASS) == HIDDEN_CLASS;
+ jboolean is_strong = (flags & STRONG_LOADER_LINK) == STRONG_LOADER_LINK;
+ jboolean vm_annotations = (flags & ACCESS_VM_ANNOTATIONS) ==
ACCESS_VM_ANNOTATION
Instead of jboolean, please use C++ bool here.
+ oop loader = lookup_k->class_loader();
+ Handle class_loader (THREAD, loader);
Can you rewrite as this to prevent potential unhandled oop for oop loader.
+ Handle class_loader (THREAD, lookup_k->class_loader());
Here:
+ InstanceKlass::cast(defined_k)->class_loader_data()->dec_keep_alive();
Don't have to cast defined_k to get class_loader_data(), but you
probably just want to move this up to remove the rest of the
InstanceKlass::cast().
+ InstanceKlass* ik = InstanceKlass::cast(defined_k);
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03/src/hotspot/share/runtime/vmStructs.cpp.udiff.html
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/classfile/ClassLoaderData.java.udiff.html
We agreed already that these changes aren't needed by the SA. You can
revert these.
These are minor changes. I don't need to see another webrev.
Thanks,
Coleen
On 3/26/20 7:57 PM, Mandy Chung wrote:
Please review the implementation of JEP 371: Hidden Classes. The
main changes are in core-libs and hotspot runtime area. Small
changes are made in javac, VM compiler (intrinsification of
Class::isHiddenClass), JFR, JDI, and jcmd. CSR [1]has been reviewed
and is in the finalized state (see specdiff and javadoc below for
reference).
Webrev:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03
Hidden class is created via `Lookup::defineHiddenClass`. From JVM's point
of view, a hidden class is a normal class except the following:
- A hidden class has no initiating class loader and is not registered
in any dictionary.
- A hidden class has a name containing an illegal character
`Class::getName` returns `p.Foo/0x1234` whereas `GetClassSignature`
returns "Lp/Foo.0x1234;".
- A hidden class is not modifiable, i.e. cannot be redefined or
retransformed. JVM TI IsModifableClass returns false on a hidden.
- Final fields in a hidden class is "final". The value of final
fields cannot be overriden via reflection. setAccessible(true) can
still be called on reflected objects representing final fields in a
hidden class and its access check will be suppressed but only have
read-access (i.e. can do Field::getXXX but not setXXX).
Brief summary of this patch:
1. A new Lookup::defineHiddenClass method is the API to create a
hidden class.
2. A new Lookup.ClassOption enum class defines NESTMATE and STRONG
option that
can be specified when creating a hidden class.
3. A new Class::isHiddenClass method tests if a class is a hidden class.
4. Field::setXXX method will throw IAE on a final field of a hidden class
regardless of the value of the accessible flag.
5. JVM_LookupDefineClass is the new JVM entry point for
Lookup::defineClass
and defineHiddenClass to create a class from the given bytes.
6. ClassLoaderData implementation is not changed. There is one
primary CLD
that holds the classes strongly referenced by its defining
loader. There
can be zero or more additional CLD
Re: Review Request: 8238358: Implementation of JEP 371: Hidden Classes
Hi, This is great work! I did a prereview and all of my comments were
addressed. These are a few minor things I noticed.
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03/src/hotspot/share/ci/ciInstanceKlass.hpp.udiff.html
Nit. Can you add 'const' to the is_hidden accessor?
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03/src/hotspot/share/classfile/classFileParser.cpp.udiff.html
+ ID annotation_index(const ClassLoaderData* loader_data, const Symbol*
name, const bool can_access_vm_annotations);
'const' bool is weird and unnecessary. Can you remove const here?
+ if (is_hidden()) { // Mark methods in hidden classes as 'hidden'.
+ m->set_hidden(true);
+ }
+
Could be:
+ // Mark methods in hidden classes as 'hidden'.
+ m->set_hidden(is_hidden());
+
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03/src/hotspot/share/classfile/javaClasses.cpp.udiff.html
+ macro(_classData_offset, k, "classData", object_signature, false); \
Probably should remove trailing backslash here.
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03/src/hotspot/share/classfile/systemDictionary.cpp.udiff.html
I think in a future RFE, we should add a default parameter to
register_loader to make the code in the beginning of parse_stream()
cleaner and remove has_class_mirror_holder_cld().
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03/src/hotspot/share/prims/jvm.cpp.udiff.html
+ jboolean is_nestmate = (flags & NESTMATE) == NESTMATE;
+ jboolean is_hidden = (flags & HIDDEN_CLASS) == HIDDEN_CLASS;
+ jboolean is_strong = (flags & STRONG_LOADER_LINK) == STRONG_LOADER_LINK;
+ jboolean vm_annotations = (flags & ACCESS_VM_ANNOTATIONS) ==
ACCESS_VM_ANNOTATION
Instead of jboolean, please use C++ bool here.
+ oop loader = lookup_k->class_loader();
+ Handle class_loader (THREAD, loader);
Can you rewrite as this to prevent potential unhandled oop for oop loader.
+ Handle class_loader (THREAD, lookup_k->class_loader());
Here:
+ InstanceKlass::cast(defined_k)->class_loader_data()->dec_keep_alive();
Don't have to cast defined_k to get class_loader_data(), but you
probably just want to move this up to remove the rest of the
InstanceKlass::cast().
+ InstanceKlass* ik = InstanceKlass::cast(defined_k);
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03/src/hotspot/share/runtime/vmStructs.cpp.udiff.html
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/classfile/ClassLoaderData.java.udiff.html
We agreed already that these changes aren't needed by the SA. You can
revert these.
These are minor changes. I don't need to see another webrev.
Thanks,
Coleen
On 3/26/20 7:57 PM, Mandy Chung wrote:
Please review the implementation of JEP 371: Hidden Classes. The main
changes are in core-libs and hotspot runtime area. Small changes are
made in javac, VM compiler (intrinsification of Class::isHiddenClass),
JFR, JDI, and jcmd. CSR [1]has been reviewed and is in the finalized
state (see specdiff and javadoc below for reference).
Webrev:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03
Hidden class is created via `Lookup::defineHiddenClass`. From JVM's point
of view, a hidden class is a normal class except the following:
- A hidden class has no initiating class loader and is not registered
in any dictionary.
- A hidden class has a name containing an illegal character
`Class::getName` returns `p.Foo/0x1234` whereas `GetClassSignature`
returns "Lp/Foo.0x1234;".
- A hidden class is not modifiable, i.e. cannot be redefined or
retransformed. JVM TI IsModifableClass returns false on a hidden.
- Final fields in a hidden class is "final". The value of final
fields cannot be overriden via reflection. setAccessible(true) can
still be called on reflected objects representing final fields in a
hidden class and its access check will be suppressed but only have
read-access (i.e. can do Field::getXXX but not setXXX).
Brief summary of this patch:
1. A new Lookup::defineHiddenClass method is the API to create a
hidden class.
2. A new Lookup.ClassOption enum class defines NESTMATE and STRONG
option that
can be specified when creating a hidden class.
3. A new Class::isHiddenClass method tests if a class is a hidden class.
4. Field::setXXX method will throw IAE on a final field of a hidden class
regardless of the value of the accessible flag.
5. JVM_LookupDefineClass is the new JVM entry point for
Lookup::defineClass
and defineHiddenClass to create a class from the given bytes.
6. ClassLoaderData implementation is not changed. There is one
primary CLD
that holds the classes strongly referenced by its defining loader.
There
can be zero or more additional CLDs - one per weak class.
7. Nest host determination is updated per revised
Re: RFR: JDK-8241618 Fix unchecked warning for jdk.hotspot.agent
On 2020-03-25 20:52, Chris Plummer wrote:
Hi Magus,
I haven't looked at the changes yet, other to see that there are many
files touched, but after reading below (and only partly understanding
since I don't know this area well), I was wondering if this issue
wouldn't be better served with multiple passes made to fix the
warnings. Start with a straight forward one where you are maybe only
making one or two types of changes, but that affect a large number of
files and don't cascade into other more complicated changes. This will
get a lot of the noise out of the way, and then we can focus on some
of the harder issues you bring up below.
Ok, I did just this. Here is an updated webrev. It contain the bulk of
the changes, but all changes are -- I dare not say trivially obvious,
but at least no-brainers. Hopefully it should be easier to review so I
can get this pushed and out of the way.
This also means that it is not possible to turn on the warning just yet.
http://cr.openjdk.java.net/~ihse/JDK-8241618-fix-unchecked-warnings-for-agent/webrev.02
/Magnus
As for testing, I think the following list will capture all of them,
but can't say for sure:
open/test/hotspot/jtreg/serviceability/sa
open/test/hotspot/jtreg/resourcehogs/serviceability/sa
open/test/jdk/sun/tools/jhsdb
open/test/jdk/sun/tools/jstack
open/test/jdk/sun/tools/jmap
open/test/hotspot/jtreg/gc/metaspace/CompressedClassSpaceSizeInJmapHeap.java
open/test/hotspot/jtreg/compiler/ciReplay/TestSAClient.java
open/test/hotspot/jtreg/compiler/ciReplay/TestSAServer.java
Chris
On 3/25/20 12:29 PM, Magnus Ihse Bursie wrote:
With the recent fixes in JDK-8241310, JDK-8237746 and JDK-8241073,
and the upcoming fixes to remove the deprecated nashorn and jdk.rmi,
the JDK build is very close to producing no warnings when compiling
the Java classes.
The one remaining sinner is jdk.hotspot.agent. Most of the warnings
here are turned off, but unchecked and deprecation cannot be
completely silenced.
Since the poor agent does not seem to receive much love nowadays, I
took it upon myself to fix these warnings, so we can finally get a
quiet build.
I started to address the unchecked warnings. Unfortunately, this was
a much bigger task than I anticipated. I had to generify most of the
module. On the plus side, the code is so much better now. And most of
the changes were trivial, just tedious.
There are a few places were I'm not entirely happy with the current
solution, and that at least merits some discussion.
I have resorted to @SuppressWarnings in four classes: ciMethodData,
MethodData, TableModelComparator and VirtualBaseConstructor. All of
them has in common that they are doing slightly fishy things with
classes in collections. I'm not entirely sure they are bug-free, but
this patch leaves the behavior untouched. I did some efforts to sort
out the logic, but it turned out to be too hairy for me to fix, and
it will probably require more substantial changes to the workings of
the code.
To make the code valid, I have moved ConstMethod to extend Metadata
instead of VMObject. My understanding is that this is benign (and
likely intended), but I really need for someone who knows the code to
confirm this. I have also added a FIXME to signal this. I'll remove
the FIXME as soon as I get confirmation that this is OK.
(The reason for this is the following piece of code from
Metadata.java: metadataConstructor.addMapping("ConstMethod",
ConstMethod.class))
In ObjectListPanel, there is some code that screams "dead" with this
change. I added a FIXME to point this out:
for (Iterator iter = elements.iterator(); iter.hasNext(); ) {
if (iter.next() instanceof Array) {
// FIXME: Does not seem possible to happen
hasArrays = true;
return;
}
It seems that if you start pulling this thread, even more dead code
will unravel, so I'm not so eager to touch this in the current patch.
But I can remove the FIXME if you want.
My first iteration of this patch tried to generify the IntervalTree
and related class hierarchy. However, this turned out to be
impossible due to some weird usage in AnnotatedMemoryPanel, where
there seemed to be confusion as to whether the tree stored
Annotations or Addresses. I'm not entirely convinced the code is
correct, it certainly looked and smelled very fishy. However, I
reverted these changes since I could not get them to work due to
this, and it was not needed for the goal of just getting rid of the
warning.
Finally, I have done no testing apart from verifying that it builds.
Please advice on suitable tests to run.
Bug: https://bugs.openjdk.java.net/browse/JDK-8241618
WebRev:
http://cr.openjdk.java.net/~ihse/JDK-8241618-fix-unchecked-warnings-for-agent/webrev.01
/Magnus
Re: Review Request: 8238358: Implementation of JEP 371: Hidden Classes
Adding back serviceability-dev. Sometimes reply (and myself) remembers
it and sometimes it strips it off
Coleen
On 3/30/20 10:16 AM, coleen.phillim...@oracle.com wrote:
On 3/29/20 10:17 PM, Mandy Chung wrote:
On 3/27/20 8:51 PM, Chris Plummer wrote:
Hi Mandy,
A couple of very minor nits in the jvmtiRedefineClasses.cpp comments:
153 // classes for primitives, arrays, hidden and vm unsafe
anonymous classes
154 // cannot be redefined. Check here so following code can
assume these classes
155 // are InstanceKlass.
156 if (!is_modifiable_class(mirror)) {
157 _res = JVMTI_ERROR_UNMODIFIABLE_CLASS;
158 return false;
159 }
I think this code and comment predate anonymous classes. Probably
before anonymous classes the check was not for
!is_modifiable_class() but instead was just a check for primitive or
array class types since they are not an InstanceKlass, and would
cause issues when cast to one in the code that lies below this
section. When anonymous classes were added, the code got changed to
use !is_modifiable_class() and the comment was not correctly updated
(anonymous classes are an InstanceKlass). Then with this webrev the
mention of hidden classes was added, also incorrectly implying they
are not an InstanceKlass. I think you should just leave off the last
sentence of the comment.
I agree with you that this comment needs update. Perhaps it should
say "primitive, array types and hidden classes are non-modifiable. A
modifiable class must be an InstanceKlass."
I may have written the last part of that comment (or remember it at
least). I think Chris's suggestion to remove the last sentence makes
sense. Anything further will just adds unnecessary confusion to the
reader. Anyone modifying this will get the InstanceKlass::cast()
assert soon after if they mess up.
Coleen
I leave it to Serguei who may have other opinion.
There's some ambiguity in the application of adjectives in the
following:
297 // Cannot redefine or retransform a hidden or an unsafe
anonymous class.
I'd suggest:
297 // Cannot redefine or retransform a hidden class or an unsafe
anonymous class.
+1
There are some places in libjdwp that need to be fixed. I spoke to
Serguei about those this afternoon. Basically the
convertSignatureToClassname() function needs to be fixed to handle
hidden classes. Without the fix classname filtering will have
problems if the filter contains a pattern with a '/' to filter on
hidden classes. Also CLASS_UNLOAD events will not properly convert
hidden class names. We also need tests for these cases. I think
these are all things that can be addressed later.
Good catch. I have created a subtask under JDK-8230502:
https://bugs.openjdk.java.net/browse/JDK-8230502
I still need to look over the JVMTI tests.
Thanks
Mandy
thanks,
Chris
On 3/26/20 4:57 PM, Mandy Chung wrote:
Please review the implementation of JEP 371: Hidden Classes. The
main changes are in core-libs and hotspot runtime area. Small
changes are made in javac, VM compiler (intrinsification of
Class::isHiddenClass), JFR, JDI, and jcmd. CSR [1]has been
reviewed and is in the finalized state (see specdiff and javadoc
below for reference).
Webrev:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03
Hidden class is created via `Lookup::defineHiddenClass`. From JVM's
point
of view, a hidden class is a normal class except the following:
- A hidden class has no initiating class loader and is not
registered in any dictionary.
- A hidden class has a name containing an illegal character
`Class::getName` returns `p.Foo/0x1234` whereas `GetClassSignature`
returns "Lp/Foo.0x1234;".
- A hidden class is not modifiable, i.e. cannot be redefined or
retransformed. JVM TI IsModifableClass returns false on a hidden.
- Final fields in a hidden class is "final". The value of final
fields cannot be overriden via reflection. setAccessible(true) can
still be called on reflected objects representing final fields in a
hidden class and its access check will be suppressed but only have
read-access (i.e. can do Field::getXXX but not setXXX).
Brief summary of this patch:
1. A new Lookup::defineHiddenClass method is the API to create a
hidden class.
2. A new Lookup.ClassOption enum class defines NESTMATE and STRONG
option that
can be specified when creating a hidden class.
3. A new Class::isHiddenClass method tests if a class is a hidden
class.
4. Field::setXXX method will throw IAE on a final field of a hidden
class
regardless of the value of the accessible flag.
5. JVM_LookupDefineClass is the new JVM entry point for
Lookup::defineClass
and defineHiddenClass to create a class from the given bytes.
6. ClassLoaderData implementation is not changed. There is one
primary CLD
that holds the classes strongly referenced by its defining
loader. There
can be zero or more additional CLDs - one
Re: Review Request: 8238358: Implementation of JEP 371: Hidden Classes
On 3/30/20 5:54 AM, David Holmes wrote:
Sorry to jump in on this but it caught my eye though I may be missing
a larger context ...
On 30/03/2020 7:30 pm, serguei.spit...@oracle.com wrote:
Hi Mandy,
I have just one comment so far.
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03/src/hotspot/share/classfile/classLoaderHierarchyDCmd.cpp.frames.html
356 void add_classes(LoadedClassInfo* first_class, int
num_classes, bool has_class_mirror_holder) {
357 LoadedClassInfo** p_list_to_add_to;
358 bool is_hidden = first_class->_klass->is_hidden();
359 if (has_class_mirror_holder) {
360 p_list_to_add_to = is_hidden ? &_hidden_weak_classes :
&_anon_classes;
361 } else {
362 p_list_to_add_to = &_classes;
363 }
364 // Search tail.
365 while ((*p_list_to_add_to) != NULL) {
366 p_list_to_add_to = &(*p_list_to_add_to)->_next;
367 }
368 *p_list_to_add_to = first_class;
369 if (has_class_mirror_holder) {
370 if (is_hidden) {
371 _num_hidden_weak_classes += num_classes;
Why does hidden imply weak here?
has_class_mirror_holder() implies weak.
Coleen
David
-
372 } else {
373 _num_anon_classes += num_classes;
374 }
375 } else {
376 _num_classes += num_classes;
377 }
378 }
Q1: I'm just curious, what happens if a cld has arrays of hidden
classes?
Is the bottom_klass always expected to be the first?
Thanks,
Serguei
On 3/26/20 16:57, Mandy Chung wrote:
Please review the implementation of JEP 371: Hidden Classes. The
main changes are in core-libs and hotspot runtime area. Small
changes are made in javac, VM compiler (intrinsification of
Class::isHiddenClass), JFR, JDI, and jcmd. CSR [1]has been reviewed
and is in the finalized state (see specdiff and javadoc below for
reference).
Webrev:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03
Hidden class is created via `Lookup::defineHiddenClass`. From JVM's
point
of view, a hidden class is a normal class except the following:
- A hidden class has no initiating class loader and is not
registered in any dictionary.
- A hidden class has a name containing an illegal character
`Class::getName` returns `p.Foo/0x1234` whereas `GetClassSignature`
returns "Lp/Foo.0x1234;".
- A hidden class is not modifiable, i.e. cannot be redefined or
retransformed. JVM TI IsModifableClass returns false on a hidden.
- Final fields in a hidden class is "final". The value of final
fields cannot be overriden via reflection. setAccessible(true) can
still be called on reflected objects representing final fields in a
hidden class and its access check will be suppressed but only have
read-access (i.e. can do Field::getXXX but not setXXX).
Brief summary of this patch:
1. A new Lookup::defineHiddenClass method is the API to create a
hidden class.
2. A new Lookup.ClassOption enum class defines NESTMATE and STRONG
option that
can be specified when creating a hidden class.
3. A new Class::isHiddenClass method tests if a class is a hidden
class.
4. Field::setXXX method will throw IAE on a final field of a hidden
class
regardless of the value of the accessible flag.
5. JVM_LookupDefineClass is the new JVM entry point for
Lookup::defineClass
and defineHiddenClass to create a class from the given bytes.
6. ClassLoaderData implementation is not changed. There is one
primary CLD
that holds the classes strongly referenced by its defining
loader. There
can be zero or more additional CLDs - one per weak class.
7. Nest host determination is updated per revised JVMS 5.4.4. Access
control
check no longer throws LinkageError but instead it will throw IAE
with
a clear message if a class fails to resolve/validate the nest
host declared
in NestHost/NestMembers attribute.
8. JFR, jcmd, JDI are updated to support hidden classes.
9. update javac LambdaToMethod as lambda proxy starts using nestmates
and generate a bridge method to desuger a method reference to a
protected
method in its supertype in a different package
This patch also updates StringConcatFactory, LambdaMetaFactory, and
LambdaForms
to use hidden classes. The webrev includes changes in nashorn to
hidden class
and I will update the webrev if JEP 372 removes it any time soon.
We uncovered a bug in Lookup::defineClass spec throws LinkageError
and intends
to have the newly created class linked. However, the implementation
in 14
does not link the class. A separate CSR [2] proposes to update the
implementation to match the spec. This patch fixes the implementation.
The spec update on JVM TI, JDI and Instrumentation will be done as
a separate RFE [3]. This patch includes new tests for JVM TI and
java.instrument that validates how the existing APIs work for hidden
classes.
javadoc/specdiff
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidd
Re: Discussion about fixing deprecation in jdk.hotspot.agent
No opinions on this? /Magnus On 2020-03-25 23:34, Magnus Ihse Bursie wrote: Hi everyone, As a follow-up to the ongoing review for JDK-8241618, I have also looked at fixing the deprecation warnings in jdk.hotspot.agent. These fall in three broad categories: * Deprecation of the boxing type constructors (e.g. "new Integer(42)"). * Deprecation of java.util.Observer and Observable. * The rest (mostly Class.newInstance(), and a few number of other odd deprecations) The first category is trivial to fix. The last category need some special discussion. But the overwhelming majority of deprecation warnings come from the use of Observer and Observable. This really dwarfs anything else, and needs to be handled first, otherwise it's hard to even spot the other issues. My analysis of the situation is that the deprecation of Observer and Observable seems a bit harsh, from the PoV of jdk.hotspot.agent. Sure, it might be limited, but I think it does exactly what is needed here. So the migration suggested in Observable (java.beans or java.util.concurrent) seems overkill. If there are genuine threading issues at play here, this assumption might be wrong, and then maybe going the j.u.c. route is correct. But if that's not, the main goal should be to stay with the current implementation. One way to do this is to sprinkle the code with @SuppressWarning. But I think a better way would be to just implement our own Observer and Observable. After all, the classes are trivial. I've made a mock-up of this solution, were I just copied the java.util.Observer and Observable, and removed the deprecation annotations. The only thing needed for the rest of the code is to make sure we import these; I've done this for three arbitrarily selected classes just to show what the change would typically look like. Here's the mock-up: http://cr.openjdk.java.net/~ihse/hotspot-agent-observer/webrev.01 Let me know what you think. /Magnus
Re: Review Request: 8238358: Implementation of JEP 371: Hidden Classes
Sorry to jump in on this but it caught my eye though I may be missing a
larger context ...
On 30/03/2020 7:30 pm, serguei.spit...@oracle.com wrote:
Hi Mandy,
I have just one comment so far.
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03/src/hotspot/share/classfile/classLoaderHierarchyDCmd.cpp.frames.html
356 void add_classes(LoadedClassInfo* first_class, int num_classes,
bool has_class_mirror_holder) {
357 LoadedClassInfo** p_list_to_add_to;
358 bool is_hidden = first_class->_klass->is_hidden();
359 if (has_class_mirror_holder) {
360 p_list_to_add_to = is_hidden ? &_hidden_weak_classes :
&_anon_classes;
361 } else {
362 p_list_to_add_to = &_classes;
363 }
364 // Search tail.
365 while ((*p_list_to_add_to) != NULL) {
366 p_list_to_add_to = &(*p_list_to_add_to)->_next;
367 }
368 *p_list_to_add_to = first_class;
369 if (has_class_mirror_holder) {
370 if (is_hidden) {
371 _num_hidden_weak_classes += num_classes;
Why does hidden imply weak here?
David
-
372 } else {
373 _num_anon_classes += num_classes;
374 }
375 } else {
376 _num_classes += num_classes;
377 }
378 }
Q1: I'm just curious, what happens if a cld has arrays of hidden classes?
Is the bottom_klass always expected to be the first?
Thanks,
Serguei
On 3/26/20 16:57, Mandy Chung wrote:
Please review the implementation of JEP 371: Hidden Classes. The main
changes are in core-libs and hotspot runtime area. Small changes are
made in javac, VM compiler (intrinsification of Class::isHiddenClass),
JFR, JDI, and jcmd. CSR [1]has been reviewed and is in the finalized
state (see specdiff and javadoc below for reference).
Webrev:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03
Hidden class is created via `Lookup::defineHiddenClass`. From JVM's point
of view, a hidden class is a normal class except the following:
- A hidden class has no initiating class loader and is not registered
in any dictionary.
- A hidden class has a name containing an illegal character
`Class::getName` returns `p.Foo/0x1234` whereas `GetClassSignature`
returns "Lp/Foo.0x1234;".
- A hidden class is not modifiable, i.e. cannot be redefined or
retransformed. JVM TI IsModifableClass returns false on a hidden.
- Final fields in a hidden class is "final". The value of final
fields cannot be overriden via reflection. setAccessible(true) can
still be called on reflected objects representing final fields in a
hidden class and its access check will be suppressed but only have
read-access (i.e. can do Field::getXXX but not setXXX).
Brief summary of this patch:
1. A new Lookup::defineHiddenClass method is the API to create a
hidden class.
2. A new Lookup.ClassOption enum class defines NESTMATE and STRONG
option that
can be specified when creating a hidden class.
3. A new Class::isHiddenClass method tests if a class is a hidden class.
4. Field::setXXX method will throw IAE on a final field of a hidden class
regardless of the value of the accessible flag.
5. JVM_LookupDefineClass is the new JVM entry point for
Lookup::defineClass
and defineHiddenClass to create a class from the given bytes.
6. ClassLoaderData implementation is not changed. There is one
primary CLD
that holds the classes strongly referenced by its defining loader.
There
can be zero or more additional CLDs - one per weak class.
7. Nest host determination is updated per revised JVMS 5.4.4. Access
control
check no longer throws LinkageError but instead it will throw IAE with
a clear message if a class fails to resolve/validate the nest host
declared
in NestHost/NestMembers attribute.
8. JFR, jcmd, JDI are updated to support hidden classes.
9. update javac LambdaToMethod as lambda proxy starts using nestmates
and generate a bridge method to desuger a method reference to a
protected
method in its supertype in a different package
This patch also updates StringConcatFactory, LambdaMetaFactory, and
LambdaForms
to use hidden classes. The webrev includes changes in nashorn to
hidden class
and I will update the webrev if JEP 372 removes it any time soon.
We uncovered a bug in Lookup::defineClass spec throws LinkageError and
intends
to have the newly created class linked. However, the implementation
in 14
does not link the class. A separate CSR [2] proposes to update the
implementation to match the spec. This patch fixes the implementation.
The spec update on JVM TI, JDI and Instrumentation will be done as
a separate RFE [3]. This patch includes new tests for JVM TI and
java.instrument that validates how the existing APIs work for hidden
classes.
javadoc/specdiff
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/api/
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/specdiff/
JVMS
Re: Review Request: 8238358: Implementation of JEP 371: Hidden Classes
Hi Mandy,
I have just one comment so far.
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03/src/hotspot/share/classfile/classLoaderHierarchyDCmd.cpp.frames.html
356 void add_classes(LoadedClassInfo* first_class, int num_classes,
bool has_class_mirror_holder) {
357 LoadedClassInfo** p_list_to_add_to;
358 bool is_hidden = first_class->_klass->is_hidden();
359 if (has_class_mirror_holder) {
360 p_list_to_add_to = is_hidden ? &_hidden_weak_classes :
&_anon_classes;
361 } else {
362 p_list_to_add_to = &_classes;
363 }
364 // Search tail.
365 while ((*p_list_to_add_to) != NULL) {
366 p_list_to_add_to = &(*p_list_to_add_to)->_next;
367 }
368 *p_list_to_add_to = first_class;
369 if (has_class_mirror_holder) {
370 if (is_hidden) {
371 _num_hidden_weak_classes += num_classes;
372 } else {
373 _num_anon_classes += num_classes;
374 }
375 } else {
376 _num_classes += num_classes;
377 }
378 }
Q1: I'm just curious, what happens if a cld has arrays of hidden classes?
Is the bottom_klass always expected to be the first?
Thanks,
Serguei
On 3/26/20 16:57, Mandy Chung wrote:
Please review the implementation of JEP 371: Hidden Classes. The main
changes are in core-libs and hotspot runtime area. Small changes are
made in javac, VM compiler (intrinsification of Class::isHiddenClass),
JFR, JDI, and jcmd. CSR [1]has been reviewed and is in the finalized
state (see specdiff and javadoc below for reference).
Webrev:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03
Hidden class is created via `Lookup::defineHiddenClass`. From JVM's point
of view, a hidden class is a normal class except the following:
- A hidden class has no initiating class loader and is not registered
in any dictionary.
- A hidden class has a name containing an illegal character
`Class::getName` returns `p.Foo/0x1234` whereas `GetClassSignature`
returns "Lp/Foo.0x1234;".
- A hidden class is not modifiable, i.e. cannot be redefined or
retransformed. JVM TI IsModifableClass returns false on a hidden.
- Final fields in a hidden class is "final". The value of final
fields cannot be overriden via reflection. setAccessible(true) can
still be called on reflected objects representing final fields in a
hidden class and its access check will be suppressed but only have
read-access (i.e. can do Field::getXXX but not setXXX).
Brief summary of this patch:
1. A new Lookup::defineHiddenClass method is the API to create a
hidden class.
2. A new Lookup.ClassOption enum class defines NESTMATE and STRONG
option that
can be specified when creating a hidden class.
3. A new Class::isHiddenClass method tests if a class is a hidden class.
4. Field::setXXX method will throw IAE on a final field of a hidden class
regardless of the value of the accessible flag.
5. JVM_LookupDefineClass is the new JVM entry point for
Lookup::defineClass
and defineHiddenClass to create a class from the given bytes.
6. ClassLoaderData implementation is not changed. There is one
primary CLD
that holds the classes strongly referenced by its defining loader.
There
can be zero or more additional CLDs - one per weak class.
7. Nest host determination is updated per revised JVMS 5.4.4. Access
control
check no longer throws LinkageError but instead it will throw IAE with
a clear message if a class fails to resolve/validate the nest host
declared
in NestHost/NestMembers attribute.
8. JFR, jcmd, JDI are updated to support hidden classes.
9. update javac LambdaToMethod as lambda proxy starts using nestmates
and generate a bridge method to desuger a method reference to a
protected
method in its supertype in a different package
This patch also updates StringConcatFactory, LambdaMetaFactory, and
LambdaForms
to use hidden classes. The webrev includes changes in nashorn to
hidden class
and I will update the webrev if JEP 372 removes it any time soon.
We uncovered a bug in Lookup::defineClass spec throws LinkageError and
intends
to have the newly created class linked. However, the implementation
in 14
does not link the class. A separate CSR [2] proposes to update the
implementation to match the spec. This patch fixes the implementation.
The spec update on JVM TI, JDI and Instrumentation will be done as
a separate RFE [3]. This patch includes new tests for JVM TI and
java.instrument that validates how the existing APIs work for hidden
classes.
javadoc/specdiff
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/api/
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/specdiff/
JVMS 5.4.4 change:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/Draft-JVMS-HiddenClasses.pdf
CSR:
https://bugs.openjdk.java.net/browse/JDK-8238359
Thanks
Mandy
[1] https://bugs.openjdk.java.net/browse/JDK-8
RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
Hi,
this is webrev.5 based on Robbin's feedback and Martin's review - thanks! :)
The change affects jvmti, hotspot and c2. Partial reviews are very welcome too.
Full: http://cr.openjdk.java.net/~rrich/webrevs/2019/8227745/webrev.5/
Delta: http://cr.openjdk.java.net/~rrich/webrevs/2019/8227745/webrev.5.inc/
Robbin, Martin, please let me know, if anything shouldn't be quite as you
wanted it. Also find my
comments on your feedback below.
Robbin, can I count you as Reviewer for the runtime part?
Thanks, Richard.
--
> DeoptimizeObjectsALotThread is only used in compileBroker.cpp.
> You can move both declaration and definition to that file, no need to clobber
> thread.[c|h]pp. (and the static function deopt_objs_alot_thread_entry)
Done.
> Does JvmtiDeferredUpdates really need to be in thread.hpp, can't be in it's
> own
> hpp file? It doesn't seem right to add JVM TI classes into thread.hpp.
I moved JvmtiDeferredUpdates to vframe_hp.hpp where preexisting
jvmtiDeferredLocalVariableSet is
declared.
> src/hotspot/share/code/compiledMethod.cpp
> Nice cleanup!
Thanks :)
> src/hotspot/share/code/debugInfoRec.cpp
> src/hotspot/share/code/debugInfoRec.hpp
> Additional parmeters. (Remark: I think "non_global_escape_in_scope" would
> read better than "not_global_escape_in_scope", but your version is consistent
> with existing code, so no change request from my side.) Ok.
I've been thinking about this too and finally stayed with
not_global_escape_in_scope. It's supposed
to mean an object whose escape state is not GlobalEscape is in scope.
> src/hotspot/share/compiler/compileBroker.cpp
> src/hotspot/share/compiler/compileBroker.hpp
> Extra thread for DeoptimizeObjectsALot. (Remark: I would have put it into a
> follow up change together with the test in order to make this webrev smaller,
> but since it is included, I'm reviewing everything at once. Not a big deal.)
> Ok.
Yes the change would be a little smaller. And if it helps I'll split it off. In
general I prefer
patches that bring along a suitable amount of tests.
> src/hotspot/share/opto/c2compiler.cpp
> Make do_escape_analysis independent of JVMCI capabilities. Nice!
It is the main goal of the enhancement. It is done for C2, but could be done
for JVMCI compilers
with just a small effort as well.
> src/hotspot/share/opto/escape.cpp
> Annotation for MachSafePointNodes. Your added functionality looks correct.
> But I'd prefer to move the bulky code out of the large function.
> I suggest to factor out something like has_not_global_escape and
> has_arg_escape. So the code could look like this:
> SafePointNode* sfn = sfn_worklist.at(next);
> sfn->set_not_global_escape_in_scope(has_not_global_escape(sfn));
> if (sfn->is_CallJava()) {
> CallJavaNode* call = sfn->as_CallJava();
> call->set_arg_escape(has_arg_escape(call));
> }
> This would also allow us to get rid of the found_..._escape_in_args variables
> making the loops better readable.
Done.
> It's kind of ugly to use strcmp to recognize uncommon trap, but that seems to
> be the way to do it (there are more such places). So it's ok.
Yeah. I copied the snippet.
> src/hotspot/share/prims/jvmtiImpl.cpp
> src/hotspot/share/prims/jvmtiImpl.hpp
> The sequence is pretty complex:
> VM_GetOrSetLocal element initialization executes EscapeBarrier code which
> suspends the target thread (extra VM Operation).
Note that the target threads have to be suspended already for
VM_GetOrSetLocal*. So it's mainly the
synchronization effect of EscapeBarrier::sync_and_suspend_one() that is
required here. Also no extra
_handshake_ is executed, since sync_and_suspend_one() will find the target
threads already
suspended.
> VM_GetOrSetLocal::doit_prologue performs object deoptimization (by VM Thread
> to prepare VM Operation with frame deoptimization).
> VM_GetOrSetLocal destructor implicitly calls EscapeBarrier destructor which
> resumes the target thread.
> But I don't have any improvement proposal. Performance is probably not a
> concern, here. So it's ok.
> VM_GetOrSetLocal::deoptimize_objects deoptimizes the top frame if it has
> non-globally escaping objects and other frames if they have arg escaping
> ones. Good.
It's not specifically the top frame, but the frame that is accessed.
> src/hotspot/share/runtime/deoptimization.cpp
> Object deoptimization. I have more comments and proposals, here.
> First of all, handling recursive and waiting locks in relock_objects is
> tricky, but looks correct.
> Comments are sufficient to understand why things are done as they are
> implemented.
> BiasedLocking related parts are complex, but we may get rid of them in the
> future (with BiasedLocking removal).
> Anyway, looks correct, too.
> Typo in comment: "regularily" => "regularly"
> Deoptimization::fetch_unroll_info_helper is the only place where
> _jvmti_deferred_updates get deallocated (except JavaThread destructor). But I
> think we always go through it, s
RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents
Hi,
this is webrev.5 based on Robbin's feedback and Martin's review - thanks! :)
The change affects jvmti, hotspot and c2. Partial reviews are very welcome too.
Full: http://cr.openjdk.java.net/~rrich/webrevs/2019/8227745/webrev.5/
Delta: http://cr.openjdk.java.net/~rrich/webrevs/2019/8227745/webrev.5.inc/
Robbin, Martin, please let me know, if anything shouldn't be quite as you
wanted it. Also find my
comments on your feedback below.
Robbin, can I count you as Reviewer for the runtime part?
Thanks, Richard.
-Original Message-
From: Doerr, Martin
Sent: Donnerstag, 12. März 2020 17:28
To: Reingruber, Richard ; 'Robbin Ehn'
; Lindenmaier, Goetz ; David
Holmes ; Vladimir Kozlov (vladimir.koz...@oracle.com)
; serviceability-dev@openjdk.java.net;
hotspot-compiler-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net
Subject: RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in
the Presence of JVMTI Agents
Hi Richard,
I managed to find time for a (almost) complete review of webrev.4. (I'll review
the tests separately.)
First of all, the change seems to be in pretty good quality for its significant
complexity. I couldn't find any real bugs. But I'd like to propose minor
improvements.
I'm convinced that it's mature because we did substantial testing.
I like the new functionality for object deoptimization. It can possibly be
reused for future escape analysis based optimizations. So I appreciate having
it available in the code base.
In addition to that, your change makes the JVMTI implementation better
integrated into the VM.
Now to the details:
src/hotspot/share/c1/c1_IR.hpp
describe_scope parameters. Ok.
src/hotspot/share/ci/ciEnv.cpp
src/hotspot/share/ci/ciEnv.hpp
Fix for JvmtiExport::can_walk_any_space() capability. Ok.
src/hotspot/share/code/compiledMethod.cpp
Nice cleanup!
src/hotspot/share/code/debugInfoRec.cpp
src/hotspot/share/code/debugInfoRec.hpp
Additional parmeters. (Remark: I think "non_global_escape_in_scope" would read
better than "not_global_escape_in_scope", but your version is consistent with
existing code, so no change request from my side.) Ok.
src/hotspot/share/code/nmethod.cpp
Nice cleanup!
src/hotspot/share/code/pcDesc.hpp
Additional parameters. Ok.
src/hotspot/share/code/scopeDesc.cpp
src/hotspot/share/code/scopeDesc.hpp
Improved implementation + additional parameters. Ok.
src/hotspot/share/compiler/compileBroker.cpp
src/hotspot/share/compiler/compileBroker.hpp
Extra thread for DeoptimizeObjectsALot. (Remark: I would have put it into a
follow up change together with the test in order to make this webrev smaller,
but since it is included, I'm reviewing everything at once. Not a big deal.) Ok.
src/hotspot/share/jvmci/jvmciCodeInstaller.cpp
Additional parameters. Ok.
src/hotspot/share/opto/c2compiler.cpp
Make do_escape_analysis independent of JVMCI capabilities. Nice!
src/hotspot/share/opto/callnode.hpp
Additional fields for MachSafePointNodes. Ok.
src/hotspot/share/opto/escape.cpp
Annotation for MachSafePointNodes. Your added functionality looks correct.
But I'd prefer to move the bulky code out of the large function.
I suggest to factor out something like has_not_global_escape and
has_arg_escape. So the code could look like this:
SafePointNode* sfn = sfn_worklist.at(next);
sfn->set_not_global_escape_in_scope(has_not_global_escape(sfn));
if (sfn->is_CallJava()) {
CallJavaNode* call = sfn->as_CallJava();
call->set_arg_escape(has_arg_escape(call));
}
This would also allow us to get rid of the found_..._escape_in_args variables
making the loops better readable.
It's kind of ugly to use strcmp to recognize uncommon trap, but that seems to
be the way to do it (there are more such places). So it's ok.
src/hotspot/share/opto/machnode.hpp
Additional fields for MachSafePointNodes. Ok.
src/hotspot/share/opto/macro.cpp
Allow elimination of non-escaping allocations. Ok.
src/hotspot/share/opto/matcher.cpp
src/hotspot/share/opto/output.cpp
Copy attribute / pass parameters. Ok.
src/hotspot/share/prims/jvmtiCodeBlobEvents.cpp
Nice cleanup!
src/hotspot/share/prims/jvmtiEnv.cpp
src/hotspot/share/prims/jvmtiEnvBase.cpp
Escape barriers + deoptimize objects for target thread. Good.
src/hotspot/share/prims/jvmtiImpl.cpp
src/hotspot/share/prims/jvmtiImpl.hpp
The sequence is pretty complex:
VM_GetOrSetLocal element initialization executes EscapeBarrier code which
suspends the target thread (extra VM Operation).
VM_GetOrSetLocal::doit_prologue performs object deoptimization (by VM Thread to
prepare VM Operation with frame deoptimization).
VM_GetOrSetLocal destructor implicitly calls EscapeBarrier destructor which
resumes the target thread.
But I don't have any improvement proposal. Performance is probably not a
concern, here. So it's ok.
VM_GetOrSetLocal::deoptimize_objects deoptimizes the top frame if it has
non-globally escaping objects and other frames if they have arg
