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

[Zoltan Somogyi]

On 15-Nov-2002, Fergus Henderson <fjh at cs.mu.OZ.AU> wrote:
> > @@ -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.
> 
> The comment above that code looks out-of-date now.

Done.

> See my earlier review comments: the pseudo-code should show when the
> "want" file gets read,

It can't, because want files never get read; only their existence matters.

> and what happens if the timeout expirese.

I added this:

	try to get mutual exclusion (i.e. create mutex file)
        if we got mutual exclusion
                check whether any want files exist
                if some do 
                        abort the timeout
                else
                        clean up, deleting the mutex file last
                        exit
                fi
        else
                some client has the lock, they need a server,
                        so abort the timeout
        fi


> > +# --optimize-duplicate-calls interacts badly with the current definition
> > +# of the mode `array_uo'.
> 
> There should be an XXX there.

Done.

> Why not use io__read_file_as_string?
> Why not use io__remove_file?

Done.

> > +:- 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.

Done.

> > +:- 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?

If e.g. a developer deletes a library function, I want them to be able to
compile the deep profiler to check whether their change screws it up.

>  What's wrong with #error here?  If the deep
> profiler is not enabled, it shouldn't be getting compiled, surely.

Not by users, but by developers.

> 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.

Done.

> > +:- 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,

Done.

> and/or the code in configure.in needs to check that these headers
> exist before enabling the deep profiler.

Will do; I believe I can assume the presence of stdio.h, but I am not sure
about stdlib.

Actually you didn't comment on the more obvious problem, which is that these
two foreign_decls should be merged into one. Also, I think we don't actually
need fcntl.h here, although at one point we did.

> > +	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.

Both done.

> > +:- 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.

Both done.

> What about `help' and `version'?

mdprof_cgi ignores its command line if QUERY_STRING is set, because web
servers can pass the query URL in strangely-chopped-up bits and pieces
on the command line. I could make mdprof_cgi respect these options
if QUERY_STRING isn't set.

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

What should we report? Of course 0.11 for the release, but we don't
want it to keep on reporting 0.11. I could call library__version,
but the deep_profiler can change version even if the library doesn't.
I can add a predicate, deep_profiler__version, which is maintained
the same way as library__version.

The last seems the least bad option. Opinions?

I also note that mmc doesn't understand --version. The same issues
apply there.

> > +main -->
> > +	io__progname_base("mdprof_tool", ProgName),
> 
> s/mdprof_tool/mdprof_test/ ?

Done.

> > +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/

Done.

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

I will leave this change until later. I don't want to change the testing
infrastructure until I have the time (and disk space) to test those changes,
and I don't right now.

> > +:- type option
> > +	--->	canonical_clique
> > +	;	flat
> > +	;	test
> > +	;	test_dir.
> 
> What about `help'?

I'll post proposed help messages soon.

> > 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.

I added:

% XXX: io__report_stats writes to the current output stream, which in 
% mdprof_cgi is the closed stream stdout. We want to write the report
% to _OutputStream, but the library doesn't support that yet. We don't
% want to set and unset the current output stream, since io__see/io__seen
% don't use a stack of I/O streams.
%
% The stats are needed only when writing the deep profiling paper anyway.

> > +% 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.

I don't think it is since Unix doesn't support mandatory locks, but I added

% (The mutex file exists iff some process holds the lock.)

to both comments.

> > @@ -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.

Done.

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

Done.

> > -char	*MP_timeout_file2;
> > -char	*MP_timeout_file3;
> > +#if defined(MR_HAVE_DIRENT_H)
> > +  #include	<sys/types.h>
> > +  #include	<dirent.h>
> > ...
> > +#else
> > +  /* if we get here, the deep profiler isn't enabled */
> > +#endif
> 
> There should be an #error in the #else branch.

As I explained above, I would prefer not to put one there.

Instead, I am making the foreign_code be empty unless we have all the features
we need, and #ifdef out the references to that foreign_code in the
foreign_procs.

> >  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.

Made this call MR_fatal_error.

> open(), close(), O_CREAT, and O_EXCL are not standard C.
> These should be #ifdef'd and/or checked for in configure.in.

Will do, for all similar comments. However, I would prefer to discuss
my proposed solutions with you in person on monday.

> > --- 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.

Will do.

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

This design is more easily expandable. For example, it should be trivial
to call MR_perform_registered_exception_cleanups not just in the Mercury
exception handler but also in the signal handlers we set up in the runtime
as well.

Zoltan.
--------------------------------------------------------------------------
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
--------------------------------------------------------------------------