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
