RFR(m): 8223306: Remove threads linked list (use ThreadsList's array in SA)

Wed May 15 13:20:07 UTC 2019

On 2019-05-15 14:59, David Holmes wrote:
>> Lambdas removed!
> 
> I got caught out by the cumulative patch file again :(
> 
> Changes look good!

Thanks David!

/Robbin

> 
>>>
>>> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/tools/StackTrace.java
>>>
>>> Existing:
>>>
>>> 76                 JavaThread cur = threads.getJavaThreadAt(k);
>>> 77                 if (cur.isJavaThread()) {
>>>
>>> How can cur possibly be anything other than a JavaThread?
>>
>> The SA seems to have a slightly different model, so only pure JavaThreads 
>> return true.
>> ServiceThread, CompilerThread, etc, returns false.
> 
> Ah - a poorly named method that emulates ThreadService::is_hidden_thread.
> 
> Thanks,
> David
> 
>>
>>>
>>> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/ui/JavaThreadsPanel.java
>>>
>>> Same issue:
>>>
>>>   463             if (t.isJavaThread()) {
>>>
>>> Possibly more of these I didn't go searching - so probably separate cleanup RFE.
>>>
>>> ---
>>>
>>> Up to you whether you want to keep lambda's but at least ensure consistency 
>>> in their use - ie use them everywhere you can. But you know my thoughts on 
>>> it. :)
>>
>> Removed!
>>
>> Thanks, Robbin.
>>
>>>
>>> Thanks,
>>> David
>>> -----
>>>
>>>>
>>>> http://cr.openjdk.java.net/~rehn/8223306/v2/inc/webrev/
>>>> http://cr.openjdk.java.net/~rehn/8223306/v2/webrev/
>>>>
>>>> /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