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

[David Holmes]

Hi Robbin,

On 8/05/2019 7:20 pm, Robbin Ehn wrote:
> Correct links:
> http://cr.openjdk.java.net/~rehn/8223306/v2/webrev/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/CommandProcessor.java.sdiff.html 

So you removed all uses of lambda in that file ... okay

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/Threads.java

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.

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?

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. :)

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