Discussion about fixing deprecation in jdk.hotspot.agent

David Holmes david.holmes at oracle.com
Wed Apr 1 23:29:10 UTC 2020 

On 2/04/2020 8:16 am, coleen.phillimore at oracle.com wrote:
> On 4/1/20 4:01 PM, Chris Plummer wrote:
>> On 4/1/20 5:13 AM, Magnus Ihse Bursie wrote:
>>>
>>>
>>> On 2020-04-01 12:22, Kevin Walls wrote:
>>>> Hi Coleen and all -
>>>>
>>>> Given the choice I'd ask that we don't remove attach/detach because 
>>>> it limits the scope of what a clhsdb can do in future. Commands like 
>>>> jstack are a one-shot operation.  A Tool like clhsdb is ideally more 
>>>> flexible than that.
>>>>
>>>> The SA is (I suggest) "too static" in its "there is one VM" 
>>>> approach, so we can't write a Tool that attaches to multiple VMs. If 
>>>> we remove attach/detach, it could not even gather information in a 
>>>> series of requests.  This is not going to be in the product any time 
>>>> soon, and maybe never, but it doesn't look right that we cut off 
>>>> such experiments.
>>>>
>>>> Removing the Observer, yes would imply making detach into detach and 
>>>> exit.  I think the clhsdb "attach" command would still work (once 
>>>> only!) but is odd without detach (so do we make "bin/jhsdb clhsdb" 
>>>> require parameters...).
>>>>
>>>> It looks like this changes the direction of the Tools in order to 
>>>> remove the deprecation warnings.
>>>>
>>>> Magnus' webrev 
>>>> http://cr.openjdk.java.net/~ihse/hotspot-agent-observer/webrev.01/ 
>>>> does add/duplicate some code, but I like it for keeping things 
>>>> working 8-)
>>>
>>> Here's an updated version of that approach that minimizes the amount 
>>> of new code:
>>>
>>> http://cr.openjdk.java.net/~ihse/hotspot-agent-observer/webrev.03
>>>
>>> The difference is that I do not duplicate the classes themselves, I 
>>> just subclass them to get a single point where the @SuppressWarnings 
>>> can be put. The only change needed in the rest of the files is to 
>>> make sure we import these classes instead of the ones in java.util.
>>>
>> HI Magnus,
>>
>> I think at this time this is probably the best approach. It's just two 
>> new SA classes that are simple subclasses of Observer and Observable, 
>> and a bunch new imports in any file that references them, right? I 
>> would just suggest adding a comment to these two classes to indicate 
>> why this is being done.
>>
> 
> When I was looking at this, most of the files import the java.util 
> version of Observer and Observable:
> 
> import java.util.Observable;
> import java.util.Observer;
> 
> If you load the subclass, then these don't have to be changed in all the 
> other files?

Yes something not right here. For example I see in

sun/jvm/hotspot/runtime/Threads.java

import java.util.*;

which is an import-on-demand statement that would allow Observer and 
Observable to be found. But then later we have:

import sun.jvm.hotspot.utilities.*;

which would be an import-on-demand statement that would allow the new 
Observer and Observable to be found.

Consequently a simple reference to Observer or Observable will be 
ambiguous and should result in an a compile-time error!

There would have to be an explicit import using

import sun.jvm.hotspot.utilities.Observable;
import sun.jvm.hotspot.utilities.Observer;

to negate the import-on-demand of java.util.*

David
----

> Coleen
> 
>> thanks,
>>
>> Chris
>>> /Magnus
>>>
>>>>
>>>>
>>>> Thanks
>>>> Kevin
>>>>
>>>>
>>>> On 31/03/2020 22:20, coleen.phillimore at oracle.com wrote:
>>>>>
>>>>>
>>>>> On 3/31/20 4:55 PM, Chris Plummer wrote:
>>>>>> On 3/31/20 1:32 PM, coleen.phillimore at oracle.com wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 3/31/20 12:19 PM, Poonam Parhar wrote:
>>>>>>>> Hello Coleen,
>>>>>>>>
>>>>>>>> Does the removal of this code only impact the 'reattach' 
>>>>>>>> functionality, and it does not affect any commands available in 
>>>>>>>> 'clhsdb' once it is attached to a core file? If that's true, 
>>>>>>>> then I think it should be okay to remove this code.
>>>>>>>
>>>>>>> Hi Poonam,  Thank you for answering. Yes, this patch only removes 
>>>>>>> the reattach functionality.  I tried out the other clhsdb 
>>>>>>> commands from your wiki page, and they worked fine, including 
>>>>>>> object and heap inspection.
>>>>>> I'm trying to understand exactly when all these static initializes 
>>>>>> are triggered. Is it only after you do an attach?
>>>>>>
>>>>>> The implementation of clhsdb reattach is exactly the same as doing 
>>>>>> a detach followed by an attach to the same process. I'm not sure 
>>>>>> how much value it has, but I think in general the removal of this 
>>>>>> code means you can't detach and then attach to anything, even a 
>>>>>> different pid. So "detach" might as well become "detach-and-exit", 
>>>>>> because your clhsdb session is dead once you detach. Do we really 
>>>>>> want to do this?
>>>>>
>>>>> Well, that was my question. It seems like you could just exit and 
>>>>> start up jhsdb again and that's more like something someone would 
>>>>> do just as easily.  Given the use cases that we've seen from 
>>>>> sustaining, this appears to be unneeded functionality.
>>>>>
>>>>> The original mail was proposing adding more code to work around the 
>>>>> deprecation messages.  It seems like more code should not be added 
>>>>> for something that is unused.
>>>>>
>>>>> thanks,
>>>>> Coleen
>>>>>
>>>>>>
>>>>>> Chris
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Coleen
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Poonam
>>>>>>>>
>>>>>>>> On 3/31/20 5:34 AM, coleen.phillimore at oracle.com wrote:
>>>>>>>>>
>>>>>>>>> To answer my own question, this functionality is used to allow 
>>>>>>>>> detach/reattach from {cl}hsdb.  Which seems to work on linux 
>>>>>>>>> but not windows with this code removed.
>>>>>>>>>
>>>>>>>>> The next question is whether this is useful functionality to 
>>>>>>>>> justify all this code (900+ and this new code that Magnus has 
>>>>>>>>> added).  Can't you just exit and restart the clhsdb process on 
>>>>>>>>> the core file or process?
>>>>>>>>>
>>>>>>>>> For the record, this is me playing with python to remove this 
>>>>>>>>> code.
>>>>>>>>>
>>>>>>>>> http://cr.openjdk.java.net/~coleenp/2020/01/webrev/index.html
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Coleen
>>>>>>>>>
>>>>>>>>> On 3/30/20 3:04 PM, coleen.phillimore at oracle.com wrote:
>>>>>>>>>>
>>>>>>>>>> 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
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>
>>
>>
>

More information about the serviceability-dev mailing list