[m-dev.] For review: coroutining changes

[Fergus Henderson]

On 25-Nov-1998, Thomas Charles CONWAY <conway at cs.mu.OZ.AU> wrote:
> Improvements to coroutining support. These changes allow us to do
> provide io primatives that cause the Mercury context to suspend rather
> than causing the engine to block.

> +++ pragma_c_gen.m	1998/11/22 22:45:41
> @@ -382,14 +382,20 @@
>  	%
>  	% Code fragments to obtain and release the global lock
>  	%
> +	code_info__get_module_info(ModuleInfo),
>  	{ ThreadSafe = thread_safe ->
>  		ObtainLock = pragma_c_raw_code(""),
>  		ReleaseLock = pragma_c_raw_code("")
>  	;
> -		ObtainLock =
> -			pragma_c_raw_code("\tMR_OBTAIN_GLOBAL_C_LOCK();\n"),
> -		ReleaseLock =
> -			pragma_c_raw_code("\tMR_RELEASE_GLOBAL_C_LOCK();\n")
> +		module_info_pred_proc_info(ModuleInfo, PredId, ProcId,
> +			PredInfo, _),

If you don't need to proc_info, call module_info_pred_info
rather than module_info_pred_proc_info.

> @@ -447,7 +453,7 @@
>  	% join all the components of the pragma_c together
>  	%
>  	{ Components = [InputComp, SaveRegsComp, ObtainLock, C_Code_Comp,
> -			CheckR1_Comp, ReleaseLock, RestoreRegsComp,
> +			ReleaseLock, CheckR1_Comp, RestoreRegsComp,
>  			OutputComp] },
>  	{ PragmaCCode = node([
>  		pragma_c(Decls, Components, MayCallMercury, MaybeFailLabel, no)

The log message doesn't mention that bug fix.

> +	/*
> +	** Check to see if any contexts that blocked on IO have become
> +	** runnable. Return the number of contexts that are still blocked.
> +	** The parameter specifies whether or not the call to select should
> +	** block or not.
> +	*/
> +static int check_pending_contexts(Bool block);
> +static int
> +check_pending_contexts(Bool block)
> +{

You should delete the first declaration of `check_pending_contexts',
and unindent the comment.

> +	int	err, n;
> +	fd_set	rd, wr, ex;
> +	struct timeval timeout;

`fd_set' and `struct timeval' are not ANSI C, so this stuff should be
#ifdef'd and autoconf'd.

I would much prefer it if you would use more meaningful variable names
throughout this function.
E.g. perhaps `max_fd' instead of `n', and perhaps `read_fds' instead
of `rd', and something instead of `tmp'.

> +	MR_PendingContext *tmp;
> +
> +	if (MR_pending_contexts == NULL)
> +		return 0;

Add { } around the if body here please.

> +	MR_fd_zero(&rd); MR_fd_zero(&wr); MR_fd_zero(&ex);
> +	n = -1;
> +	for (tmp = MR_pending_contexts ; tmp ; tmp = tmp -> next) {

s/ ;/;/g

> +		if (tmp->waiting_mode & MR_PENDING_READ) {
> +			n = ((n > tmp->fd) ? n : tmp->fd);

That would be simpler if written as

			if (n > tmp->fd) n = tmp->fd;

or
			n = MR_MAX(n, tmp->fd);

> +	if (n == 0)
> +		fatal_error("no fd's set!");

Please use { } here.

> +	if (block) {
> +		err = select(n, &rd, &wr, &ex, NULL);
> +	} else {
> +		timeout.tv_sec = 0;
> +		timeout.tv_usec = 0;
> +		err = select(n, &rd, &wr, &ex, &timeout);
> +	}
> +
> +	if (err < 0)
> +		fatal_error("select failed!");

Likewise.

> +	n = 0;
> +	for (tmp = MR_pending_contexts ; tmp ; tmp = tmp -> next) {
> +		n++;
> +		if (tmp->waiting_mode & MR_PENDING_READ) {
> +			if(FD_ISSET(tmp->fd, &rd)) {
> +				schedule(tmp->context);
> +				continue;
> +			}
> +		}
> +		if (tmp->waiting_mode & MR_PENDING_WRITE) {
> +			if(FD_ISSET(tmp->fd, &wr)) {
> +				schedule(tmp->context);
> +				continue;
> +			}
> +		}
> +		if (tmp->waiting_mode & MR_PENDING_EXEC) {
> +			if(FD_ISSET(tmp->fd, &ex)) {
> +				schedule(tmp->context);
> +				continue;
> +			}
> +		}

IMHO this would be better written using `&&' rather than nested ifs:

		if ((tmp->waiting_mode & MR_PENDING_WRITE)
		    && FD_ISSET(tmp->fd, &wr))
		{
			schedule(tmp->context);
			continue;
		}
		if (...

It might also be better to use "else if" rather than "continue":

		if ((tmp->waiting_mode & MR_PENDING_WRITE)
			&& FD_ISSET(tmp->fd, &wr))
		{
			schedule(tmp->context);
		} else if ((tmp->waiting_mode & MR_PENDING_READ)
			&& FD_ISSET(tmp->fd, &rd))
		{
			schedule(tmp->context);
		} else if ((tmp->waiting_mode & MR_PENDING_EXEC)
			&& FD_ISSET(tmp->fd, &ex))
		{
			schedule(tmp->context);
		}

Or in fact to use `||':

		if (	((tmp->waiting_mode & MR_PENDING_WRITE)
				&& FD_ISSET(tmp->fd, &wr))
		   ||	((tmp->waiting_mode & MR_PENDING_READ)
				&& FD_ISSET(tmp->fd, &rd))
		   ||	((tmp->waiting_mode & MR_PENDING_EXEC)
				&& FD_ISSET(tmp->fd, &ex))
		   )
		{
			schedule(tmp->context);
		}

Yep, on reflection I like this one best.

> -			if (depth > 0 && tmp->owner_thread == thd
> -					|| tmp->owner_thread == NULL)
> +			if ((depth > 0 && tmp->owner_thread == thd)
> +				|| (tmp->owner_thread == (MercuryThread) NULL))
>  				break;

Use { ... } around the if body, please.

	if (...
		|| ...)
	{
		break;
	}
	
> -	if (MR_runqueue_head == NULL)
> +	if (MR_runqueue_head == NULL && MR_pending_contexts == NULL)
>  		fatal_error("empty runqueue!");
> +
> +	while (MR_runqueue_head == NULL)
> +		check_pending_contexts(TRUE); /* block */

Use { } around if and while bodies, please.

> +/*
> +** As well as the runqueue, we maintain a linked list of contexts
> +** and associated file descriptors that are suspended blocked for
> +** reads/writes/exceptions. When the runqueue becomes empty, if
> +** this list is not empty then we call select and block until one
> +** or more of the file descriptors become ready for I/O, then
> +** wake the appropriate context.
> +** In addition, we periodically check to see if the list of blocked
> +** contexts is non-empty and if so, poll to wake any contexts that
> +** can unblock. This, while not yielding true fairness (since this
> +** requires the current context to perform some yield-like action),
> +** ensures that it is possible for programmers to write concurrent
> +** programs with continuous computation and interleaved I/O dependent
> +** computation in a straight-forward manner.
> +*/

Is the second paragraph implemented yet?  If not, you should make
that clear.

> +#define	MR_PENDING_READ		0x01
> +#define	MR_PENDING_WRITE	0x02
> +#define	MR_PENDING_EXEC		0x04

How about

	enum {
		MR_PENDING_READ  = 0x01,
		MR_PENDING_WRITE = 0x02,
		MR_PENDING_EXEC  = 0x04
	};

?
In general it's better to avoid #define if possible.

> -  #define MR_OBTAIN_GLOBAL_C_LOCK()	MR_mutex_lock(&MR_global_lock, \
> -						"pragma c code");
> +  #define MR_OBTAIN_GLOBAL_C_LOCK(where)	MR_LOCK(&MR_global_lock, \
> +							where);
>  
> -  #define MR_RELEASE_GLOBAL_C_LOCK()	MR_mutex_unlock(&MR_global_lock, \
> -						"pragma c code");
> +  #define MR_RELEASE_GLOBAL_C_LOCK(where)	MR_UNLOCK(&MR_global_lock, \
> +							where);

All uses of the macro parameter `where' should be in parentheses.

> -begin-mercury_select.c----------------------------------------------------
> #include "mercury_select.h"
> 
> void
> MR_fd_zero(fd_set *fdset)
> {
> 	FD_ZERO(fdset);
> }
> 
> -end-mercury_select.c----------------------------------------------------
> -begin-mercury_select.h----------------------------------------------------
> 
> #ifndef	MR_MERCURY_SELECT_H
> #define MR_MERCURY_SELECT_H
> 
> #include <sys/time.h>
> #include <sys/types.h>
> #include <unistd.h>
> 
> void MR_fd_zero(fd_set *fdset);
> 
> #endif
> -end-mercury_select.h----------------------------------------------------

Documentation, please!

That stuff should probably go in mercury_misc.{c.h}, alongside
`MR_memcpy()', since it exists for very similar reasons.

It is also non-portable and hence should be #ifdef'd and autoconf'd.

----------

There were a number of changes which were not documented in the log message,
including the changes to mercury_types.h and mercury_library_types.h.

Apart from the points raised above, it looks fine.
But I think it needs another round of reviewing.

Cheers,
	Fergus.

-- 
Fergus Henderson <fjh at cs.mu.oz.au>  |  "Binaries may die
WWW: <http://www.cs.mu.oz.au/~fjh>  |   but source code lives forever"
PGP: finger fjh at 128.250.37.3        |     -- leaked Microsoft memo.