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

Robbin Ehn robbin.ehn at oracle.com
Tue May 14 14:02:36 UTC 2019


Hi Dan,

Full:
http://cr.openjdk.java.net/~rehn/8223306/v3/webrev/index.html
Inc:
http://cr.openjdk.java.net/~rehn/8223306/v3/inc/webrev/index.html

On 2019-05-08 18:02, Daniel D. Daugherty wrote:
> General comment - Please make sure to update all copyright years before
>                    pushing this changeset.

Fixed.

> 
> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/Threads.java
>      L46:
>      L47:     private static AddressField      threadsField;
>      L48:     private static CIntegerField     lengthField;
>          nit - please delete blank line on L46
>          nit - please reduce the space between type and variable names
>                (I have no preference if you still keep them aligned)
> 
>      nit - Please delete blank line on L74.

Fixed.


> 
> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/tools/PStack.java
>      old L203:       Threads threads = VM.getVM().getThreads();
>      old L204:       for (JavaThread cur = threads.first(); cur != null; cur = 
> cur.next()) {
>      new L203:       VM.getVM().getThreads().doJavaThreads((cur) -> {
>          In this case, you did a lambda conversion...
> 
> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/tools/StackTrace.java
>      old L75:             for (JavaThread cur = threads.first(); cur != null; 
> cur = cur.next(), i++) {
>      new L75:             for (int k = 0; k < threads.getNumberOfThreads(); k++) {
>      new L76:                 JavaThread cur = threads.getJavaThreadAt(k);
>          In this case, you didn't do a lambda conversion...
> 
>          I'm trying to grok a reason for the different styles...
>          Update: Is is maybe a control flow thing? (No, I don't know much
>             about lambdas.) As in: Loops that "break" or early return are
>             not amenable to conversion to a lambda... (just guessing)

I converted those where it was easy to see that the loop did not have an early 
termination. Lambdas removed.

> 
>      L74:             int i = 1;
>          Not your bug, but I think that 'i' is not used.
> 

Fixed.

> This all looks good to me... so thumbs up!

Thanks.

> 
> I have some reservations about using lambdas in a debugging tool.
> My personal philosophy about debugging tools is that they should
> be built on the simplest/most stable technology to reduce the
> chances of the more complicated technology failing during a debug
> session. I hate it when my debugger crashes!

I removed all lambdas!

Thanks for looking at this, Robbin

> 
> That said, SA is pretty much standalone so use of lambdas in this
> debugging tool shouldn't affect the JVM or core file being debugged.
> 
> Again, thumbs up!
> 
> Dan
> 
> 
>>
>> /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
> 


More information about the hotspot-runtime-dev mailing list