Re: Patch: initialize ProcessManager early

[David Daney <ddaney at avtrex dot com>]

Tom Tromey wrote:
> This patch fixes the process -vs- InheritableThreadLocal problem I was
> seeing.  It works by initializing the ProcessManager thread early.
>
> This should let us complete mauve testing.
>
> Mark, can you try this with frysk?  If it works I will check it in on
> the RH 4.1 branch.
>
> Tom
>
> Index: ChangeLog
> from  Tom Tromey  <tromey@redhat.com>
>
> 	* gnu/java/lang/natMainThread.cc (call_main): Initialize platform
> 	Process class.
> 	* java/lang/PosixProcess.java (ProcessManager.run): Never stop
> 	thread.
> 	(PosixProcess): Don't start ProcessManager.
> 	Start ProcessManager in static initializer.

Tom,

You asked me to comment here after our IRC conversation.

I don't like this patch.  It is bad for two reasons:

1) Programs that never directly or indirectly do Runtime.exec() are now 
burdened with the weight of an extra thread.

2) Startup time for all programs will now be slower because this extra 
thread is created sooner that it used to be.

David Daney


> Index: gnu/java/lang/natMainThread.cc
> ===================================================================
> --- gnu/java/lang/natMainThread.cc	(revision 122011)
> +++ gnu/java/lang/natMainThread.cc	(working copy)
> @@ -1,6 +1,6 @@
>  // natMainThread.cc - Implementation of MainThread native methods.
>  
> -/* Copyright (C) 1998, 1999, 2000, 2001, 2003, 2006  Free Software Foundation
> +/* Copyright (C) 1998, 1999, 2000, 2001, 2003, 2006, 2007  Free Software Foundation
>  
>     This file is part of libgcj.
>  
> @@ -17,15 +17,30 @@
>  #include <jvm.h>
>  #include <java-threads.h>
>  
> +#include <platform.h>
> +
>  #include <gnu/java/lang/MainThread.h>
>  #include <java/lang/Runtime.h>
>  #include <java/lang/ThreadGroup.h>
>  
> +// It is convenient and safe to simply include all of these.
> +#include <java/lang/Win32Process.h>
> +#include <java/lang/EcosProcess.h>
> +#include <java/lang/PosixProcess.h>
> +
>  typedef void main_func (jobject);
>  
>  void
>  gnu::java::lang::MainThread::call_main (void)
>  {
> +  // Initialize the process manager here.  This is only needed on
> +  // POSIX systems, but it is simpler to do it everywhere.  We do this
> +  // so that the process manager thread is stated before any
> +  // InheritableThreadLocals are made.  Otherwise if an
> +  // InheritableThreadLocal needs a stack trace we can get into
> +  // infinite recursion trying to run addr2line.
> +  _Jv_InitClass (&_Jv_platform_process::class$);
> +
>    Utf8Const* main_signature = _Jv_makeUtf8Const ("([Ljava.lang.String;)V", 22);
>    Utf8Const* main_name = _Jv_makeUtf8Const ("main", 4);
>  
> Index: java/lang/PosixProcess.java
> ===================================================================
> --- java/lang/PosixProcess.java	(revision 122011)
> +++ java/lang/PosixProcess.java	(working copy)
> @@ -1,5 +1,5 @@
>  // PosixProcess.java - Subclass of Process for POSIX systems.
> -/* Copyright (C) 1998, 1999, 2004, 2006  Free Software Foundation
> +/* Copyright (C) 1998, 1999, 2004, 2006, 2007  Free Software Foundation
>  
>     This file is part of libgcj.
>  
> @@ -127,27 +127,7 @@
>  	    {
>  	      synchronized (queueLock)
>  	        {
> -		  boolean haveMoreChildren = reap();
> -		  if (! haveMoreChildren && queue.size() == 0)
> -		    {
> -		      // This reaper thread could exit, but we
> -		      // keep it alive for a while in case
> -		      // someone wants to start more Processes.
> -		      try
> -		        {
> -			  queueLock.wait(1000L);
> -			  if (queue.size() == 0)
> -			    {
> -			      processManager = null;
> -			      return; // Timed out.
> -			    }
> -		        }
> -		      catch (InterruptedException ie)
> -		        {
> -			  // Ignore and exit the thread.
> -			  return;
> -		        }
> -		    }
> +		  reap();
>  		  while (queue.size() > 0)
>  		    {
>  		      PosixProcess p = (PosixProcess) queue.remove(0);
> @@ -361,16 +341,8 @@
>      this.envp = envp;
>      this.dir = dir;
>  
> -    // Start a ProcessManager if there is not one already running.
>      synchronized (queueLock)
>        {
> -	if (processManager == null)
> -	  {
> -	    processManager = new ProcessManager();
> -	    processManager.start();
> -	    processManager.waitUntilReady();
> -	  }
> -
>  	// Queue this PosixProcess for starting by the ProcessManager.
>  	processManager.startExecuting(this);
>        }
> @@ -457,6 +429,13 @@
>    private static Object queueLock = new Object();
>    private static ProcessManager processManager;
>  
> +  static
> +  {
> +    processManager = new ProcessManager();
> +    processManager.start();
> +    processManager.waitUntilReady();
> +  }
> +
>    static class EOFInputStream extends InputStream
>    {
>      static EOFInputStream instance = new EOFInputStream();