[Mono-dev] [PATCHES] Improve process exit/wait handling

[Rolf Bjarne Kvinge]

Hi Rodrigo,

>Hi Rolf,
>
>Reviewing 0004-io-layer-Improve-waiting-on-processes-a-lot.patch:
>
>
>+	// Ensure we're not in here in multiple threads at once, nor
recursive.
>+	if (InterlockedIncrement (&mono_processes_cleaning_up) > 1) {
>+		InterlockedDecrement (&mono_processes_cleaning_up);
>+		
>Use the standard idiom of CAS (&zzz, 0, 1) instead of doing 2 atomic ops.

Fixed.

>+			if (mono_processes_soft_lock != 0) {
>+				/* The sigchld handler is watching us. Spin
a bit and try again */
>+				_wapi_handle_spin (1);
>+			} else {
>
>This code doesn't make much sense to me. You spin with
the mono_processes_mutex lock held
>only to drop it later to reacquire straight away. You must use an
exponential backoff since the other
>thread might be blocked for a significant amount of time.

I changed to spin for a max of 7ms, and if not successful then just give up.

>Overall the patch is ok, thou I have two comments, first that the style of
reclamation is kludgy, we
>would be better with SMR; second that, if I understand correctly, we only
free process data when its
>managed object is collected or another process is spawned, right?

Yes, we free process data is only when another process is spawned. The
problem resides in choosing a better moment: it can obviously only happen
after the process has exited, and it can't happen in the signal handler. I
don't think it's that important to free those bits as soon as possible - and
it won't grow to a huge amount of memory either (the user would have to
start a lot of processes, and then stop creating processes before any of the
already created processes exits).

It might probably be an idea to free the process data upon shutdown though,
at least to make valgrind happy, though I'm not sure where to do that.
 
>On other notes, your code mixes C and C++ styles comments, stick with C
style, please. You're
>not following our code conventions in a few places too:
>
>+static void mono_processes_cleanup (void)
>
>Should be:
>static void
>mono_processes_cleanup (void)

Fixed.

Thanks a lot,
Rolf


________________________________________
No virus found in this message.
Checked by AVG - www.avg.com
Version: 10.0.1204 / Virus Database: 1498/3511 - Release Date: 03/16/11
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-io-layer-Improve-waiting-on-processes-a-lot.patch
Type: application/octet-stream
Size: 24195 bytes
Desc: not available
Url : http://lists.ximian.com/pipermail/mono-devel-list/attachments/20110318/65673271/attachment-0001.obj