Discussion about fixing deprecation in jdk.hotspot.agent

Chris Plummer chris.plummer at oracle.com
Wed Apr 1 20:01:15 UTC 2020 

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.

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