Ping: Re: RFR: JDK-8241618 Fix unchecked warning for jdk.hotspot.agent

Wed Apr 15 07:02:27 UTC 2020

On 2020-04-15 02:34, serguei.spitsyn at 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
>>>>>
>>>>>
>>>>
>>>
>>
>