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

[Robbin Ehn]

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