RFR(m): 8223306: Remove threads linked list (use ThreadsList's array in SA)
Robbin Ehn
robbin.ehn at oracle.com
Thu May 16 06:28:56 UTC 2019
Thanks Serguei!
/Robbin
On 2019-05-16 02:10, serguei.spitsyn at oracle.com wrote:
> 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 hotspot-runtime-dev
mailing list