Discussion about fixing deprecation in jdk.hotspot.agent

Magnus Ihse Bursie magnus.ihse.bursie at oracle.com
Wed Apr 1 12:13:59 UTC 2020



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.

/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