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

[Robbin Ehn]

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