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

[David Holmes]

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