Hi David,
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-09 06:46, David Holmes wrote:
As Dan also hints at you are a bit inconsistent with keeping loops or replacing
with lambda's. Anything that would do a "break" or "return" in the loop can't be
converted to a lambda; but "continue" becomes "return" in the lambda version.
Overall I'd just say forget about any internal iteration using lambdas and just
make the simple changes needed for the loop version. It's a less disruptive
change and doesn't add the complexity and overhead of lambda expressions.
Lambdas removed!
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.
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
