Hi Cheleswer,
Looks good to me.
Thanks
Kevin
(Also, as one of the comments was that there may be no real cost to
using default stack sizes here (on most systems...?), having
jdk.lang.processReaperUseDefaultStackSize gives us a way to test that
widely, and one day the 32k may be able to disappear.)
On 17/02/2016 15:17, cheleswer sahu wrote:
Hi,
I have made changes in the property name
(jdk.lang.processReaperUseDefaultStackSize) and code as suggested in
the earlier reviews.
--- old/src/java.base/share/classes/java/lang/ProcessHandleImpl.java
2016-02-17 18:48:10.545639999 +0530
+++ new/src/java.base/share/classes/java/lang/ProcessHandleImpl.java
2016-02-17 18:48:10.081639999 +0530
@@ -81,9 +81,8 @@
ThreadGroup systemThreadGroup = tg;
ThreadFactory threadFactory = grimReaper -> {
- // Our thread stack requirement is quite modest.
- Thread t = new Thread(systemThreadGroup, grimReaper,
- "process reaper", 32768);
+ long stackSize =
Boolean.getBoolean("jdk.lang.processReaperUseDefaultStackSize") ? 0 :
32768;
+ Thread t = new Thread(systemThreadGroup,
grimReaper, "process reaper", stackSize);
t.setDaemon(true);
// A small attempt (probably futile) to avoid
priority inversion
t.setPriority(Thread.MAX_PRIORITY);
I would really like to thanks "Martin" for the patch
(http://cr.openjdk.java.net/~martin/webrevs/openjdk9/tls-size-guarantee/)
which he has provided. Since we support a wider range of glibc
versions and platform using patch will
require more testing and work. I think the use of system property for
this issue will be safer at this point of time.
Regards,
Cheleswer
On 1/19/2016 5:40 PM, David Holmes wrote:
On 19/01/2016 9:53 PM, Kevin Walls wrote:
|
Hi Cheleswer, I think Martin is suggesting something like:
|
// Use a modest stack size, unless requested otherwise:
long stackSize =
Boolean.getBoolean("processReaperUseDefaultStackSize") ? 0 : 32768;
Someone from core-libs needs to chime on what the appropriate
namespace for such a property would be.
David
-----
Thread t = new Thread(systemThreadGroup, grimReaper, "process
reaper", stackSize);
|||
If that tests OK for you, it may be the way we can go ahead with the
point fix for this.
Regards
Kevin
|
On 18/01/2016 16:52, Martin Buchholz wrote:
Historical note - I never liked having a reaper thread for each
subprocess, but no other reliable implementation is known. Given the
potential for many reaper threads, I at least wanted to keep the
memory waste low.
On Wed, Jan 13, 2016 at 2:25 AM, cheleswer sahu
<cheleswer.s...@oracle.com> wrote:
+ Thread t = null;
+ if
(Boolean.getBoolean("processReaperUseDefaultStackSize")) {
+ t = new Thread(systemThreadGroup, grimReaper,
"process reaper");
+ } else {
+ // Our thread stack requirement is quite
modest.
+ t = new Thread(systemThreadGroup, grimReaper,
"process reaper", 32768);
+ }
If we do end up using this strategy, cleaner would be to use
https://docs.oracle.com/javase/8/docs/api/java/lang/Thread.html#Thread-java.lang.ThreadGroup-java.lang.Runnable-java.lang.String-long-
stackSize - the desired stack size for the new thread, or zero to
indicate that this parameter is to be ignored.
