Discussion about fixing deprecation in jdk.hotspot.agent

Chris Plummer chris.plummer at oracle.com
Thu Apr 2 00:21:51 UTC 2020 

On 4/1/20 4:29 PM, David Holmes wrote:
> 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.*
Magnus only fixed 3 of the SA files as an example. I think the plan is 
to add the above import to every file that uses Observer/Observable. But 
as you point out, some already import java.util.Observable/Observable, 
and you can't have both, so in that case the new import should replace 
the old one.

Chris
>
> 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