On 2020-04-15 02:34, serguei.spit...@oracle.com wrote:
Hi Magnus,
It looks good to me.
Thanks for the review, Serguei!
/Magnus
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,
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<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
