For the record this looks fine to me too.
Thanks,
David
-----
On 18/02/2016 1:17 AM, 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/%7Emartin/webrevs/openjdk9/tls-size-guarantee/>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.
