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

[coleen.phillimore at oracle.com]

Looks good to me.
Coleen

On 5/8/19 5:20 AM, 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 
>
>
> 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