RFR(m): 8223306: Remove threads linked list (use ThreadsList's array in SA)
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Thu May 16 00:10:20 UTC 2019
Hi Robbin,
Looks good to me.
Thanks,
Serguei
On 5/14/19 07:02, 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
>
> 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
>>
More information about the serviceability-dev
mailing list