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.
Unfortunately, many changes tends to cling together -- for instance,
class Foo has a List fooList of say Integer. If I change that to
List, then also the constructor needs to change, and the
getFooList() method, and that in turn propagate to users of getFooList()
etc. I tried to do this piecewise but for every line that I fixed I just
ended up getting more and more places that needed fixing.
On the other hand, the patch I present *is* indeed mostly trivial. Apart
from the places I mentioned below, the fixes are straightforward. And I
opted out of fixing the tricky ones by disabling the warnings. My
intention is to file a follow-up bug for these @SuppressWarnings to be
fixed properly. However, doing that is unfortunately beyond the scope of
what I'm able to do, since I do not have enough domain knowledge. The
fixes in this patch is more or less "stupid" applications of adding
generics with the correct type. (Basically, what I've done is to locate
a problematic type, like fooList, and check the type of elements
inserted and extracted of it, and created it as a generic of that type.
Boring, but not really difficult.)
I realize the webrev can look daunting. Perhaps start by looking at the
patch file, that will quickly show what kind of changes this is about.
Also, 1/3 of the patch is just about updating those darned copyright
years. :-(
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.
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
Thank you! I'll run these through our test system.
/Magnus
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
Re: RFR: JDK-8241463 Move build tools to respective modules
Hi Magnus, On 3/24/20 4:16 AM, Magnus Ihse Bursie wrote: At the core, we'd like to "invert" the current structure where we have files like: make/lib/Lib-java.base.gmk make/lib/Lib-java.desktop.gmk make/gensrc/Gensrc-java.base.gmk make/gensrc/Gensrc-java.desktop.gmk ... etc and instead have like: make/modules/java.base/lib.gmk make/modules/java.base/gensrc.gmk make/modules/java.desktop/lib.gmk make/modules/java.desktop/gensrc.gmk : We already have collected everything else that belongs to a module under src/$module/share. Apart from classes, and native, we have: * conf * lib * legal * man for those modules that require them. My suggestion is that we add, for those module that require them: * data * tools I think we should take at modularizing make/data, build tools, make/gensrc etc as a whole and put down other options considered. I haven't had time looking closely but I try to understand how make/data are used. Here are some examples of it that produces either gensrc/$MODULE/$PACKAGE/*.java files or files in jdk build output. - generate source files from gensrc/$MODULE/$PACKAGE/*.java.template - generate resource bundle from gensrc/$MODULE/$PACKAGE/resource/*.properties - generate CLDR locale data from make/data/cldr files - generate jdk/lib/tzdb.dat from make/data/tzdata - generate jdk/lib/ct.sym from make/data/symbol - generate jdk/lib/security/cacerts from make/data/cacerts - generate jdk/lib/security/backlisted.certs from make/data/blacklistedcertsconverter - generate jdk/lib/security/public_suffix_list.data from make/data/publicsuffixlist I can see why you propose the data files are moved to the source. There could be other options to explore. Another observation: Some build tools are module-specific (e.g. generate icons .png.java files) that is clearly used only by one module. There are other build tools that can be used by any module e.g. generate resource bundle .java source. And regarding JEP 201, as far as I can tell the additional "man" and "lib" directories do not seem to be reflected in JEP 201. I do not know if this is considered an oversight, or just reflecting the fact that the directory structure will continue to evolve after JEP 201 were delivered. But if you think JEP 201 needs to be updated, fine, I'll gladly help with that. JEP 299 discusses the reorganization of man pages and specification and specifies the man directory. When JEP 201 was written, the man directory was not present (part of moving the specs effort to the openjdk source). It was added in JDK 12. "lib" directory is an oversight. JEP 201 may need update for this build modularization work too. Mandy
Re: RFR: JDK-8241618 Fix unchecked warning for jdk.hotspot.agent
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.
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
RFR: JDK-8241618 Fix unchecked warning for jdk.hotspot.agent
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: RFR: (T) 8241144 Javadoc is not generated for new module jdk.nio.mapmode
On 2020-03-25 02:22, David Holmes wrote: On 25/03/2020 3:49 am, Florian Weimer wrote: * Magnus Ihse Bursie: On 2020-03-24 09:59, Andrew Dinn wrote: On 23/03/2020 18:38, Erik Joelsson wrote: Looks good. Thanks for the review, Erik. I'm assuming that also implies it is trivial (because, copyright update a side, it really is a 1-liner :-). For code in the build system, we do not have the Hotspot rules of multiple reviewers, waiting period or trtiviality. A single OK review is enough to be allowed to push it. Where are these rules documented? I looked for them on openjdk.java.net, but could not find them unfortunately. Hotspot rules are buried in here: https://wiki.openjdk.java.net/display/HotSpot/HotSpot+How+To Thanks for the link, David. For build code, we don't have any such set of rules, so the absence of rules kind of is the rules. The rule about at least one reviewer is enforced by the JDK project (and jcheck), but that's about it. Hopefully, with Project Skara, many rules such as these can be enforced and/or informed about automatically with bots. /Magnus "Before pushing" You must be a Committer in the JDK project You need a non-JEP JBS issue for tracking Your change must have been available for review at least 24 hours to accommodate for all time zones Your change must have been approved by two Committers out of which at least one is also a Reviewer Your change must have passed through the hs tier 1 testing provided by the submit-hs repository with zero failures You must run all relevant testing to make sure your actual change is working You must be available the next few hours, and the next day and ready to follow up with any fix needed in case your change causes problems in later tiers There is a notion of trivial changes that can be pushed sooner than 24 hours. It should be clearly stated in the review mail that the intention is to push as a trivial change. How to actually define "trivial" is decided on a case-by-case basis but in general it would be things like fixing a comment, or moving code without changing it. Backing out a change is also considered trivial as the change itself in that case is generated by mercurial. Cheers, David
