Hi,
As other option, we can replace Observe/Observable to other class /interface.
AFAICS most of classes which call VM.registerVMInitializedObserver() seems not
to use arguments in Observable.
For example:
VM.registerVMInitializedObserver(() ->
initialize(VM.getVM().getTypeDataBase()));
or
VM.registerVMInitializedObserver(this::initialize); // `initialize` is method
reference: Caller would pass TypeDataBase
I know it would be bigger change than Magnus's proposal (+ modifying import
declarations), but I think it is easier to understand.
Thanks,
Yasumasa
On 2020/04/01 21:13, 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.
/Magnus
Thanks
Kevin
On 31/03/2020 22:20, coleen.phillim...@oracle.com wrote:
On 3/31/20 4:55 PM, Chris Plummer wrote:
On 3/31/20 1:32 PM, coleen.phillim...@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.phillim...@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.phillim...@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
