RFR(m): 8223306: Remove threads linked list (use ThreadsList's array in SA)
Robbin Ehn
robbin.ehn at oracle.com
Thu May 16 06:32:08 UTC 2019
Thanks Coleen!
On 2019-05-15 17:52, coleen.phillimore at oracle.com wrote:
>
> Still looks good. I guess we'll not have any lambdas to cut and paste in the SA
> now.
>
> Do you no longer need this?
>
> 191 public interface JavaThreadsDo {
> 192 void doJavaThread(JavaThread thread);
> 193 }
> 194
> 195 public void doJavaThreads(JavaThreadsDo jtDo) {
> 196 for (int i = 0; i < _list.length(); i++) {
> 197 jtDo.doJavaThread(getJavaThreadAt(i));
> 198 }
> 199 }
> 200
>
>
> If you remove it, or not, I don't need to see this webrev again.
It was removed here if you look at inc:
http://cr.openjdk.java.net/~rehn/8223306/v3/inc/webrev/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/Threads.java.sdiff.html
/Robbin
>
> Coleen
>
> On 5/15/19 9:20 AM, Robbin Ehn wrote:
>> On 2019-05-15 14:59, David Holmes wrote:
>>>> Lambdas removed!
>>>
>>> I got caught out by the cumulative patch file again :(
>>>
>>> Changes look good!
>>
>> Thanks David!
>>
>> /Robbin
>>
>>>
>>>>>
>>>>> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/tools/StackTrace.java
>>>>>
>>>>> Existing:
>>>>>
>>>>> 76 JavaThread cur = threads.getJavaThreadAt(k);
>>>>> 77 if (cur.isJavaThread()) {
>>>>>
>>>>> How can cur possibly be anything other than a JavaThread?
>>>>
>>>> The SA seems to have a slightly different model, so only pure JavaThreads
>>>> return true.
>>>> ServiceThread, CompilerThread, etc, returns false.
>>>
>>> Ah - a poorly named method that emulates ThreadService::is_hidden_thread.
>>>
>>> Thanks,
>>> David
>>>
>>>>
>>>>>
>>>>> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/ui/JavaThreadsPanel.java
>>>>>
>>>>> Same issue:
>>>>>
>>>>> 463 if (t.isJavaThread()) {
>>>>>
>>>>> Possibly more of these I didn't go searching - so probably separate cleanup
>>>>> RFE.
>>>>>
>>>>> ---
>>>>>
>>>>> Up to you whether you want to keep lambda's but at least ensure consistency
>>>>> in their use - ie use them everywhere you can. But you know my thoughts on
>>>>> it. :)
>>>>
>>>> Removed!
>>>>
>>>> Thanks, Robbin.
>>>>
>>>>>
>>>>> Thanks,
>>>>> David
>>>>> -----
>>>>>
>>>>>>
>>>>>> 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
>
More information about the hotspot-runtime-dev
mailing list