[m-rev.] for review: make the deep profiler work again

[Fergus Henderson]

On 14-Nov-2002, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:
> Make the deep profiler work again.
...
> Index: configure.in
...
> @@ -1831,7 +1831,13 @@
>      # Currently it only distinguishes between systems which have
>      # unistd.h or not, but at a later date we may also need to test for
>      # other posix features.
> -if test "$MR_HAVE_UNISTD_H" = 1; then
> +if test "$MR_HAVE_UNISTD_H" = 1 && \
> +	test "$MR_HAVE_DIRENT_H" = 1 && \
> +	test "$ac_cv_func_getpid" = yes && \
> +	test "$ac_cv_func_opendir" = yes && \
> +	test "$ac_cv_func_readdir" = yes && \
> +	test "$ac_cv_func_closedir" = yes
> +then

The comment above that code looks out-of-date now.

> +The following is a high level description of mdprof_cgi in pseudocode:
> +
> +	create "want" file
> +	get mutual exclusion (i.e. create mutex file)
> +	if the named pipes exist
> +		commit to being a client
> +		send the query to the server through the toserver pipe
> +		release mutual exclusion (i.e. delete mutex file)
> +		receive the result from the server through the fromserver pipe
> +		remove "want" file
> +	else
> +		read the profile data file and preprocess it
> +		if the reading and preprocessing found any errors
> +			report the error 
> +			release mutual exclusion (i.e. delete mutex file)
> +			remove "want" file
> +		else
> +			report the result of the initial query
> +			commit to being a server
> +			create the named pipes
> +			release mutual exclusion (i.e. delete mutex file)
> +			remove "want" file
> +			loop forever
> +				setup timeout
> +				receive query on toserver pipe
> +				send result to fromserver pipe
> +			done
> +		fi
> +	fi

See my earlier review comments: the pseudo-code should show when the
"want" file gets read, and what happens if the timeout expirese.

> Index: deep_profiler/Mercury.options
...
> +# --optimize-duplicate-calls interacts badly with the current definition
> +# of the mode `array_uo'.

There should be an XXX there.

> Index: deep_profiler/conf.m
> +		( { TmpRes = ok(TmpStream) } ->
> +			io__read_file(TmpStream, TmpReadRes),
> +			{ TmpReadRes = ok(ServerNameChars0) ->
>  				(
> -					{ list__remove_suffix(ServerNameChars0,
> -						['\n'], ServerNameChars) }
> +					list__remove_suffix(ServerNameChars0,
> +						['\n'], ServerNameChars)
>  				->
> +					string__from_char_list(
> +						ServerNameChars, ServerName)

Why not use io__read_file_as_string?

>  				;
> -					{ error("malformed server name") }
> +					error("malformed server name")
>  				)
>  			;
> -				{ error("cannot read server's name") }
> -			)
> +				error("cannot read server's name")
> +			},
> +			io__close_input(TmpStream)
>  		;
>  			{ error("cannot open file to out the server's name") }
> -		)
> +		),
> +		{ RmTmpCmd = string__format("rm %s", [s(TmpFile)]) },
> +		io__call_system(RmTmpCmd, _)

Why not use io__remove_file?

> +:- pragma foreign_decl("C",
> +"
> +#ifdef	MR_HAVE_UNISTD_H
> +#include	<sys/types.h>
> +#include	<unistd.h>
> +#endif
> +").

This should use MR_HAVE_SYS_TYPES_H.

> +:- pragma foreign_proc("C",
> +	getpid = (Pid::out),
> +	[will_not_call_mercury, promise_pure],
> +"
> +#if	defined(MR_HAVE_UNISTD_H) && defined(MR_HAVE_GETPID)
> +	Pid = getpid();
> +#else
> +	/*
> +	** If MR_HAVE_GETPID is not defined, the deep profiler is not enabled
> +	** anyway, so what the code here does doesn't matter. We still want to
> +	** make it compile cleanly, though.
> +	*/
> +	Pid = 0;

Why is it necessary that the deep profiler compile cleanly in cases
when it won't be enabled?  What's wrong with #error here?  If the deep
profiler is not enabled, it shouldn't be getting compiled, surely.

If for some reason it does need to be compiled, it would still be
better to call MR_fatal_error(), in case that code does end up
getting called.

> Index: deep_profiler/mdprof_cgi.m
...
> +:- pragma foreign_decl("C", "
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <unistd.h>
> +").
> +
> +:- pragma foreign_decl("C", "
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <fcntl.h>
> +").

These includes need to be protected with #ifdefs,
and/or the code in configure.in needs to check that these headers
exist before enabling the deep profiler.
Checking for <unistd.h> is not sufficient to guarantee existence
of these other headers.

> +:- pred check_for_existing_fifos(string::in, string::in, int::out,
> +	io__state::di, io__state::uo) is det.
> +
> +:- pragma foreign_proc("C",
> +	check_for_existing_fifos(Fifo1::in, Fifo2::in, FifoCount::out,
> +		S0::di, S::uo),
> +	[will_not_call_mercury, promise_pure, tabled_for_io],
> +"
> +	struct stat	statbuf;
> +	int		status;
> +
> +	FifoCount = 0;
> +	status = stat(Fifo1, &statbuf);
> +	if ((status == 0) && (S_ISFIFO(statbuf.st_mode))) {
> +		FifoCount++;
> +	}
> +	status = stat(Fifo2, &statbuf);
> +	if ((status == 0) && (S_ISFIFO(statbuf.st_mode))) {
> +		FifoCount++;
> +	}

That code needs to be protected with one or more #ifdefs
and/or the test in configure.in needs to be modified.

> +:- pragma foreign_proc("C",
> +	raw_detach_process(ResCode::out, S0::di, S::uo),
> +	[will_not_call_mercury, promise_pure],
> +"{
> +	pid_t	status;
> +
> +	fflush(stdout);
> +	fflush(stderr);
> +	status = fork();
> +	if (status < 0) {
> +		ResCode = -1;
> +	} else if (status > 0) {
> +		ResCode = 1;
> +	} else {
> +#ifdef	MR_HAVE_SETPGID
> +		/* detach the server process from the parent's process group */
> +		setpgid(0, 0);
> +#else
> +		/* hope that web server doesn't depend on the process group */
> +#endif
> +		ResCode = 0;
> +	}
> +
> +	S = S0;
> +}").

That code needs to be protected with one or more #ifdefs
and/or the test in configure.in needs to be modified.
Checking for unistd.h is not sufficient to guarantee
existing of struct stat or fork() -- for example, Mingw
has unistd.h but it does not have fork().

> +:- type option
> +	--->	canonical_clique
> +	;	clique
> +	;	debug
> +	;	detach_process
> +	;	modules
> +	;	proc
> +	;	quit
> +	;	root
> +	;	record_startup
> +	;	record_loop
> +	;	server_process
> +	;	timeout
> +	;	write_query_string.

What about `help' and `version'?

(`version' is particularly important in this case since
this file will get installed in a non-version-specific directory.)

> Index: deep_profiler/mdprof_test.m
...
> +main -->
> +	io__progname_base("mdprof_tool", ProgName),

s/mdprof_tool/mdprof_test/ ?

> +test_server(DirName, Pref, Deep) -->
> +	{ string__format("test -d %s || mkdir -p %s",
> +		[s(DirName), s(DirName)], Cmd) },
> +	io__call_system(Cmd, _),
...
> +:- pred write_test_html(string::in, string::in, int::in, string::in,
> +	io__state::di, io__state::uo) is det.
> +
> +write_test_html(DirName, BaseName, Num, HTML) -->
> +	% For large programs such as the Mercury compiler, the profiler data
> +	% file may contain hundreds of thousands of cliques. We therefore put
> +	% each batch of pages in a different subdirectory, thus limiting the
> +	% number of files/subdirs in each directory.
> +	{ Bunch = (Num - 1) // 1000 },
> +	{ string__format("%s/%s_%04d",
> +		[s(DirName), s(BaseName), i(Bunch)], BunchName) },
> +	( { (Num - 1) rem 1000 = 0 } ->
> +		{ string__format("test -p %s || mkdir -p %s",
> +			[s(BunchName), s(BunchName)], Cmd) },
> +		io__call_system(Cmd, _)

s/test -p/test -d/

In fact, this code should be extracted out into a subroutine
(called e.g. "make_directory_and_parent_dirs").

> +:- type option
> +	--->	canonical_clique
> +	;	flat
> +	;	test
> +	;	test_dir.

What about `help'?

> Index: deep_profiler/startup.m
> +:- pred maybe_report_stats(maybe(io__output_stream)::in,
> +	io__state::di, io__state::uo) is det.
> +
> +maybe_report_stats(yes(_)) --> [].
> +	% io__report_stats("standard"). XXX
> +maybe_report_stats(no) --> [].

The XXX comment should explain why this is commented out.

> Index: deep_profiler/timeout.m
...
> +% Get the lock on the named mutex file if the bool is `no'.
> +% If the bool is `yes', meaning debugging is enabled, do nothing.
> +
> +:- pred get_lock(bool::in, string::in,
> +	io__state::di, io__state::uo) is det.

Does this acquire a lock on the named file, thus preventing other
threads/processes from accessing it, or does it use (the existence or
nonexistence of) the named file to implement a mutex lock?
If the latter, then IMHO the comment is misleading.

> +% Release the lock on the named mutex file if the bool is `no'.
> +% If the bool is `yes', meaning debugging is enabled, do nothing.
> +
> +:- pred release_lock(bool::in, string::in,
> +	io__state::di, io__state::uo) is det.

Likewise.

> @@ -51,29 +83,199 @@
>  
>  :- pragma foreign_decl("C",
>  "
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <fcntl.h>

These should be protected with #ifdefs
and/or tested for in configure.in.

>  #include <unistd.h>	/* for alarm() */

Likewise.

>  :- pragma foreign_code("C",
>  "
> -MR_bool	MP_process_is_detached_server = MR_FALSE;
> -char	*MP_timeout_file1;
> -char	*MP_timeout_file2;
> -char	*MP_timeout_file3;
> +#if defined(MR_HAVE_DIRENT_H)
> +  #include	<sys/types.h>
> +  #include	<dirent.h>

MR_HAVE_SYS_TYPES_H

> +#else
> +  /* if we get here, the deep profiler isn't enabled */
> +#endif

There should be an #error in the #else branch.

>  void
> +MP_handle_timeout(void)
>  {
> +#if defined(MR_HAVE_DIRENT_H) && defined(MR_HAVE_OPENDIR) \
> +		&& defined(MR_HAVE_READDIR) && defined(MR_HAVE_CLOSEDIR)
...
> +#else
> +	/* if we get here, the deep profiler isn't enabled */
> +#endif
> +}

Likewise.

> +MR_bool
> +MP_do_try_get_lock(const char *mutex_file)
> +{
> +	int	res;
> +	MR_bool	success;
> +
> +	res = open(mutex_file, O_CREAT | O_EXCL, 0);
> +	if (res >= 0) {
> +		(void) close(res);
> +		MP_register_cleanup_file(mutex_file);
> +		success = MR_TRUE;
> +	} else if (res < 0 && errno == EEXIST) {

open(), close(), O_CREAT, and O_EXCL are not standard C.
These should be #ifdef'd and/or checked for in configure.in.
Likewise for EEXIST (you can use the recently added
MR_is_eexist() for that one).

> +MP_do_get_lock(const char *mutex_file)
>  {
> -	MP_delete_timeout_files();
> -	exit(EXIT_SUCCESS);
> +	int	res;
> +
> +	for (;;) {
> +		res = open(mutex_file, O_CREAT | O_EXCL, 0);
> +		if (res >= 0) {
> +			(void) close(res);
> +			MP_register_cleanup_file(mutex_file);
> +			return;
> +		} else if (res < 0 && errno == EEXIST) {
> +			sleep(5);
> +			continue;
> +		} else {
> +			MR_fatal_error(""MP_do_get_lock failed"");
> +		}
> +	}

Likewise here.

Also there are portability issues with sleep(),
see trace/mercury_trace_source.c.

>  :- pragma foreign_proc("C",
> -	setup_timeout(Minutes::in, IO0::di, IO::uo),
> +	make_want_file(WantFileName::in, S0::di, S::uo),
>  	[will_not_call_mercury, promise_pure],
>  "
> -	int	seconds;
> +	int	fd;
>  
> -	seconds = Minutes * 60;
> -	(void) alarm(seconds);
> -	IO = IO0;
> +	fd = open(WantFileName, O_CREAT, 0);
> +	if (fd < 0) {
> +		MR_fatal_error(""make_want_file: open failed"");
> +	}
> +	(void) close(fd);
> +	MP_register_cleanup_file(WantFileName);
> +	S = S0;
> +").

Likewise here.

> +:- pragma foreign_proc("C",
> +	remove_want_file(WantFileName::in, S0::di, S::uo),
> +	[will_not_call_mercury, promise_pure],
> +"
> +	MP_unregister_cleanup_file(WantFileName);
> +	(void) unlink(WantFileName);
> +	S = S0;
>  ").

Likewise here.

> Index: runtime/mercury_misc.h
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/runtime/mercury_misc.h,v
> retrieving revision 1.22
> diff -u -b -r1.22 mercury_misc.h
> --- runtime/mercury_misc.h	18 Feb 2002 07:01:18 -0000	1.22
> +++ runtime/mercury_misc.h	13 Nov 2002 12:05:53 -0000
> @@ -27,4 +27,9 @@
>  
>  extern	void	MR_fatal_error(const char *msg, ...) MR_NO_RETURN;
>  
> +extern	void	MR_register_exception_cleanup(void (*func)(void *),
> +			void *data);
> +
> +extern	void	MR_perform_registered_exception_cleanups(void);

These routines should be documented.

Why are these needed?  Why not just use a Mercury exception handler
(perhaps one that calls a C function)?

-- 
Fergus Henderson <fjh at cs.mu.oz.au>  |  "I have always known that the pursuit
The University of Melbourne         |  of excellence is a lethal habit"
WWW: <http://www.cs.mu.oz.au/~fjh>  |     -- the last words of T. S. Garp.
--------------------------------------------------------------------------
mercury-reviews mailing list
post:  mercury-reviews at cs.mu.oz.au
administrative address: owner-mercury-reviews at cs.mu.oz.au
unsubscribe: Address: mercury-reviews-request at cs.mu.oz.au Message: unsubscribe
subscribe:   Address: mercury-reviews-request at cs.mu.oz.au Message: subscribe
--------------------------------------------------------------------------