Hi Magnus,
It looks good to me.
Thanks,
Serguei
On 4/14/20 14:23, Chris Plummer wrote:
It looks good to me.
Thanks,
Serguei
On 4/14/20 14:23, Chris Plummer wrote:
Hi Magnus,
The changes look good. Just one minor issue:
http://cr.openjdk.java.net/~ihse/JDK-8241618-fix-unchecked-warnings-for-agent/webrev.02/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/Metadata.java.frames.html
Copyright was updated, but no other changes in this file.
BTW, do you know why there were cases in the old code of having the right generic type in a comment. For example:
1177 private static List/*<Field>*/ getInstanceFields(InstanceKlass ik) {
and
1276 private Map classDataCache = new HashMap(); // <InstanceKlass, ClassData>
It's almost as if some of the code was initially properly written for generics, but then backported to work with a pre-generics version of jdk.
thanks,
Chris
On 4/14/20 2:24 AM, Magnus Ihse Bursie wrote:
Hi,
Can I please get a review for this, simplified version of the patch? This only contain trivial changes, like this:
- private List objects; // ArrayList<ScopeValue> + private List<ObjectValue> objects;Basically all changes are to the container types List or Map (and a few changes from Class to Class<?>).
If the list of all the files look horrible in the webrev, have a look at the patch file instead, it is easier to scroll through and check the changes:
http://cr.openjdk.java.net/~ihse/JDK-8241618-fix-unchecked-warnings-for-agent/webrev.02/open.patch
/Magnus
On 2020-03-30 16:25, Magnus Ihse Bursie wrote:
On 2020-03-25 20:52, Chris Plummer wrote:
Hi Magus,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.
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.
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<Oop> 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
