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

Wed May 15 12:59:00 UTC 2019

Hi Robbin,

On 15/05/2019 12:08 am, Robbin Ehn wrote:
> Hi David,
> 
> 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-09 06:46, David Holmes wrote:
>> As Dan also hints at you are a bit inconsistent with keeping loops or 
>> replacing with lambda's. Anything that would do a "break" or "return" 
>> in the loop can't be converted to a lambda; but "continue" becomes 
>> "return" in the lambda version.
>>
>> Overall I'd just say forget about any internal iteration using lambdas 
>> and just make the simple changes needed for the loop version. It's a 
>> less disruptive change and doesn't add the complexity and overhead of 
>> lambda expressions.
> 
> Lambdas removed!

I got caught out by the cumulative patch file again :(

Changes look good!

>>
>> 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