Discussion about fixing deprecation in jdk.hotspot.agent
David Holmes
david.holmes at oracle.com
Thu Apr 2 00:25:07 UTC 2020
On 2/04/2020 10:21 am, Chris Plummer wrote:
> 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.
Okay - that was not at all clear from the email.
Thanks,
David
-----
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