Discussion about fixing deprecation in jdk.hotspot.agent
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Wed Apr 1 22:16:13 UTC 2020
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?
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