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.phillim...@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.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
