RFR(m): 8223306: Remove threads linked list (use ThreadsList's array in SA)
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Wed May 15 15:52:34 UTC 2019
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.
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20190515/6fdcc3cd/attachment.html>
More information about the serviceability-dev
mailing list