Hi Dan,
Fixed below, thanks!
/Robbin
On 2019-05-20 20:20, Daniel D. Daugherty wrote:
On 5/14/19 10:02 AM, Robbin Ehn wrote:
Hi Dan,
Full:
http://cr.openjdk.java.net/~rehn/8223306/v3/webrev/index.html
Inc:
http://cr.openjdk.java.net/~rehn/8223306/v3/inc/webrev/index.html
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/debugger/bsd/BsdDebuggerLocal.java
No comments.
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/ObjectHeap.java
No comments.
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/DeadlockDetector.java
L2: * Copyright (c) 2004, 20019, Oracle and/or its affiliates. All rights
reserved.
Typo - s/20019/2019/
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/JavaThread.java
No comments.
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/Threads.java
No comments.
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/tools/PStack.java
No comments.
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/tools/StackTrace.java
No comments.
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/ui/JavaThreadsPanel.java
No comments.
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/AbstractHeapGraphWriter.java
L136: writeJavaThread(jt, i+1);
formatting: s/i+1/i + 1/
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/PointerFinder.java
No comments.
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/ReversePtrsAnalysis.java
No comments.
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/soql/JSJavaVM.java
L2: * Copyright (c) 2004, 20019, Oracle and/or its affiliates. All rights
reserved.
Typo - s/20019/2019/
Thumbs up! I don't need to see a new webrev for the above nits.
I took a quick pass thru:
http://cr.openjdk.java.net/~rehn/8223306/v3/webrev/index.html
and nothing jumped out at me...
Dan
On 2019-05-08 18:02, Daniel D. Daugherty wrote:
General comment - Please make sure to update all copyright years before
pushing this changeset.
Fixed.
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/Threads.java
L46:
L47: private static AddressField threadsField;
L48: private static CIntegerField lengthField;
nit - please delete blank line on L46
nit - please reduce the space between type and variable names
(I have no preference if you still keep them aligned)
nit - Please delete blank line on L74.
Fixed.
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/tools/PStack.java
old L203: Threads threads = VM.getVM().getThreads();
old L204: for (JavaThread cur = threads.first(); cur != null; cur
= cur.next()) {
new L203: VM.getVM().getThreads().doJavaThreads((cur) -> {
In this case, you did a lambda conversion...
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/tools/StackTrace.java
old L75: for (JavaThread cur = threads.first(); cur != null;
cur = cur.next(), i++) {
new L75: for (int k = 0; k < threads.getNumberOfThreads();
k++) {
new L76: JavaThread cur = threads.getJavaThreadAt(k);
In this case, you didn't do a lambda conversion...
I'm trying to grok a reason for the different styles...
Update: Is is maybe a control flow thing? (No, I don't know much
about lambdas.) As in: Loops that "break" or early return are
not amenable to conversion to a lambda... (just guessing)
I converted those where it was easy to see that the loop did not have an early
termination. Lambdas removed.
L74: int i = 1;
Not your bug, but I think that 'i' is not used.
Fixed.
This all looks good to me... so thumbs up!
Thanks.
I have some reservations about using lambdas in a debugging tool.
My personal philosophy about debugging tools is that they should
be built on the simplest/most stable technology to reduce the
chances of the more complicated technology failing during a debug
session. I hate it when my debugger crashes!
I removed all lambdas!
Thanks for looking at this, Robbin
That said, SA is pretty much standalone so use of lambdas in this
debugging tool shouldn't affect the JVM or core file being debugged.
Again, thumbs up!
Dan
/Robbin
On 2019-05-08 11:17, Robbin Ehn wrote:
Hi David,
I changed to normal for:
http://rehn-ws.se.oracle.com/cr_mirror/8223306/v2/webrev/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/CommandProcessor.java.sdiff.html
Full:
http://rehn-ws.se.oracle.com/cr_mirror/8223306/v2/webrev/
Inc:
http://rehn-ws.se.oracle.com/cr_mirror/8223306/v2/inc/webrev/
Passes t1-2
Thanks, Robbin
On 2019-05-07 09:47, David Holmes wrote:
Hi Robbin,
On 7/05/2019 4:50 pm, Robbin Ehn wrote:
Hi David,
On 5/7/19 12:40 AM, David Holmes wrote:
Hi Robbin,
I have a few concerns here.
First I can't see how you are actually integrating the SA with the
ThreadSMR. You've exposed the _java_thread_list for it to iterate but
IIRC that list can be updated when threads are added/removed and I'm not
seeing how the SA is left iterating a valid list - we'd normally using a
ThreadsListHandle for that ?? (I may need a refresher on how this list
is actually maintained.)
The processes must be paused. If the processes would be running the
linked list is also broken since if we unlink and delete a JavaThread and
then later SA follows that _next pointer.
Ah good point. Thanks for clarifying.
The conversion from external iteration of the list (the for loop) to
internal iteration (passing a lambda to JavaThreadsDo) is also
problematic. First I'd be very wary about introducing lambda expressions
into the SA code - lambda drags in a lot of supporting code that could
have an impact on the way SA functions. There are places where we have
to avoid lambdas due to bootstrapping/initialization issues and I think
the SA may be an area where we also want to keep the code extremely simple.
There are already several usages of lambdas in SA code, e.g.
LinuxDebuggerLocal.java. SA is not a core module, it's an application,
there is not a bootstrap issue or so.
Hmm okay. Lambda carries a lot of baggage. But if its already being used ...
Second by using lambda's with internal iteration you've lost the ability
to terminate the iteration loop! In the existing code if we have a
return in the for-loop then we not only terminate the loop but the
enclosing method. With the lambda the "return" just ends the current
iteration and JavaThreadsDo will then continue with the next thread - so
we don't even terminate the iteration let alone the method performing
the iteration. So for places were we want to process one thread, for
example, we will continue to iterate all remaining threads and just do
nothing with them. That's very inefficient.
That's why I only used the internal iteration where we didn't have early
returns.
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/CommandProcessor.java
- original code:
1556 new Command("where", "where { -a | id }", false) {
1557 public void doit(Tokens t) {
...
1564 for (JavaThread thread = threads.first(); thread
!= null; thread = thread.next()) {
1565 ByteArrayOutputStream bos = new
ByteArrayOutputStream();
1566 thread.printThreadIDOn(new PrintStream(bos));
1567 if (all || bos.toString().equals(name)) {
1568 out.println("Thread " + bos.toString() +
" Address: " + thread.getAddress());
...
1577 }
1578 if (!all) return;
That looks like an early return to me.
Cheers,
David
-----
Thanks, Robbin
Thanks,
David
On 6/05/2019 5:31 pm, Robbin Ehn wrote:
Hi, please review.
Old threads linked list remove and updated SA to use ThreadsList array
instead.
Issue:
https://bugs.openjdk.java.net/browse/JDK-8223306
Webrev:
http://cr.openjdk.java.net/~rehn/8223306/webrev/
Passes t1-3 (which includes SA tests).
Thanks, Robbin
