[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR(m): 8223306: Remove threads linked list (use ThreadsList's array in SA)
From: Robbin Ehn <robbin.ehn () oracle ! com>
Date: 2019-05-21 7:52:39
Message-ID: 988a7b22-19cf-b060-7fae-a91851869592 () oracle ! com
[Download RAW message or body]
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
> > >
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic