[m-rev.] Updated diff for deep profiling.

Zoltan Somogyi zs at cs.mu.OZ.AU
Wed May 30 15:02:03 AEST 2001


On 29-May-2001, Fergus Henderson <fjh at cs.mu.OZ.AU> wrote:
> Might as well just write
> 	map__set(VarTypes0, CSNVar, int_type, VarTypes),

Done.

> The second occurrence of `ground(shared, none)' should be replaced by `Ground'

Done.

> > -	option_neg_implies(use_activation_counts,
> > -		deep_profile_tail_recursion, bool(no)),
> > +	% XXX
> > +	% option_neg_implies(use_activation_counts,
> > +	% 	deep_profile_tail_recursion, bool(no)),
> 
> There should be a comment here explaining what the XXX is for
> and why this code is commented out.

--no-use-activation-counts *used* to be incompatible with
deep_profile_tail_recursion, but I have fixed that. I removed those three lines
even before your review, so they are not in the relative diff. I posted the
revised diff while the bootcheck proving the compatibility of
--no-use-activation-counts with deep_profile_tail_recursion was in progress.


> > +		io__write_string("Usage: mdprof_cgi")
> 
> There should be a newline after the error message.
> 
> Also the message probably ought to go to stderr.

I added the newline to that error message and to another.

I know that the web server reads the stdout of the CGI scripts it invokes,
because that's how the page is given to the web server. I don't know whether
the web server reads the CGI script's stderr. Does anyone have any
documentation on that?

> > +	char	buf[1000];
> > +
> > +	sprintf(buf, ""echo %s %s %s > /tmp/timeout"",
> > +		MP_timeout_file1, MP_timeout_file2, MP_timeout_file3);
> > +	system(buf);
> 
> If this code was just for debugging, then it should be deleted.

I have.

> This should use the ANSI/ISO C remove() function
> rather than the Posix unlink().

Done.

> This is pretty ugly.

I agree, but it reduced the overhead of deep profiling from a factor of 3 to
2.7, which is good enough reason.

> It will also have some costs, even for code which does not use
> deep profiling, since those procedures and types (and the corresponding
> unification/compare procedures) will get included in the shared versions
> of the standard library even in non-.prof_deep grades.
> Why not just use an array?

Because there is no simple way to generate static arrays in the compiler.

However, I am working on a way to preserve tail recursion with deep profiling
that won't need those types, won't need the code duplication and should be
even more efficient. However, I am not yet sure whether it will work.

> > -  #ifndef MR_USE_ACTIVATION_COUNTS
> > -	MR_fatal_error(""create_proc_dynamic_inner called when not using activation counts!"");
> > -  #else
> > +  /* #ifndef MR_USE_ACTIVATION_COUNTS */
> > +	/* MR_fatal_error(""inner_call_port_code called when not using activation counts!""); */
> > +  /* #else */
> 
> Why is that check commented out?

Same reason as the implication above. I have removed the comments even
before your review.

> > +:- pragma foreign_proc("C", save_recursion_depth_3(CSD::in, CSNsVector::in,
> > +		OuterCount1::out, OuterCount2::out, OuterCount3::out),
> > +		[thread_safe, will_not_call_mercury],
> > +"{
> > +/* shut up warning: CSD, CSNsVector, OuterCount1, OuterCount2, OuterCount3 */
> > +#define MR_PROCNAME		""save_recursion_depth_3""
> > +#define MR_REC_DEPTH_BODY	{					     \
> > +				MR_SAVE_DEPTH_ACTION(OuterCount1,	     \
> > +					MR_csn_vector_field(CSNsVector, 0)); \
> > +				MR_SAVE_DEPTH_ACTION(OuterCount2,	     \
> > +					MR_csn_vector_field(CSNsVector, 1)); \
> > +				MR_SAVE_DEPTH_ACTION(OuterCount3,	     \
> > +					MR_csn_vector_field(CSNsVector, 2)); \
> > +				}
> > +#include ""mercury_deep_rec_depth_body.h""
> > +#undef MR_PROCNAME
> > +#undef MR_REC_DEPTH_BODY
> > +}").
> 
> You didn't include mercury_deep_rec_depth_body.h in the log message
> or in the full diff that you posted.
> Likewise for mercury_deep_rec_depth_actions.h.

Sorry. That's a consequence of adding them while disconnected from CVS.
They are at the end of the message.

> > +++ new/mercury/runtime/mercury_deep_profiling.c	Tue May 29 16:13:26 2001
> > @@ -13,12 +13,14 @@
> >  #include "mercury_imp.h"
> >  #include "mercury_ho_call.h"
> >  #include "mercury_stack_layout.h"
> > +#include "mercury_timing.h"
> >  #include "mercury_prof_time.h"
> >  #include "mercury_deep_profiling.h"
> >  
> >  #ifdef MR_DEEP_PROFILING
> >  
> >  #include <stdio.h>
> > +#include <unistd.h>
> 
> <unistd.h> is not an ANSI/ISO C standard header file.
> That should be wrapped inside #ifdef HAVE_UNISTD_H,
> or even better, deleted.

Done.

> > @@ -192,7 +196,7 @@
> >  	kind_csd, kind_pd, kind_css, kind_ps
> >  } MR_NodeKind;
> >  
> > -/* must correspond to fixed_size_int_bytes in deep_profiler/deep.io.m */
> > +/* must correspond to fixed_size_int_bytes in deep/read_profile.m */
> 
> "deep/" should be "deep_profiler/".
> 
> (Same occurs once in the log message.)

Done.

> 		MR_fatal_error("cannot open `Deep.data' for writing: %s",
> 			strerror(errno));

Done.

> You should use MR_warning() and strerror()
> rather than sprintf() and perror().

Done.

> > +	if (unlink(MR_MDPROF_DATAFILENAME) != 0) {
> > +		sprintf(buf, "%s: %s", "unlink", MR_MDPROF_DATAFILENAME);
> > +		perror(buf);
> > +	}
> 
> That should use remove() rather than unlink().

Done.

> > -	/* This string must match id_string deep_profiler/deep.io.m */
> > +	/* This string must match id_string deep/read_profile.m */
> 
> s/deep/deep_profiler/

Done.

> I think the code duplication in table_builtin.m should be avoided or at
> least reduced.

You mean profiling_builtin.m. I agree, but that is for a later change.

> Apart from that, and the comments above, this looks OK.
> But I'd like to see the two files that you didn't include in this diff.


::::::::::::::
mercury_deep_rec_depth_actions.h
::::::::::::::
/*
** Copyright (C) 1994-2001 The University of Melbourne.
** This file may only be copied under the terms of the GNU Library General
** Public License - see the file COPYING.LIB in the Mercury distribution.
*/

/*
** This file provides macros that library/profiling_builtin.m uses to compose
** the code fragments passed to mercury_deep_rec_depth_body.h to create the
** bodies of
**	save_recursion_depth{1..9}
**	restore_recursion_depth_exit{1..9}
**	restore_recursion_depth_fail{1..9}
**
** These macros assume that the environments of their invocation define
** the following variables:
** 
** pd:	should point to the ProcDynamic structure of the caller.
** ps: 	should point to the ProcStatic structure of the caller.
*/

#define	MR_SAVE_DEPTH_ACTION(outer_count, csn)				\
	do {								\
		MR_CallSiteDynamic	*inner_csd;			\
									\
		MR_deep_assert(csn <= ps->MR_ps_num_call_sites);	\
		inner_csd = pd->MR_pd_call_site_ptr_ptrs[csn];		\
									\
		if (inner_csd != NULL) {				\
			outer_count = inner_csd->MR_csd_depth_count;	\
		} else {						\
			outer_count = 0;				\
		}							\
	} while (0)

#define	MR_RESTORE_DEPTH_ACTION(inc_field, outer_count, csn)		\
	do {								\
		MR_CallSiteDynamic	*inner_csd;			\
									\
		MR_deep_assert(csn <= ps->MR_ps_num_call_sites);	\
		inner_csd = pd->MR_pd_call_site_ptr_ptrs[csn];		\
									\
		if (inner_csd != NULL) {				\
			inner_count = inner_csd->MR_csd_depth_count;	\
			/* calls are computed from the other counts */	\
			inner_csd->MR_csd_own.inc_field += inner_count;	\
			inner_csd->MR_csd_depth_count = outer_count;	\
		} else {						\
			MR_deep_assert(outer_count == 0);		\
		}							\
	} while (0)

#define	MR_RESTORE_DEPTH_EXIT(outer_count, csn)				\
	MR_RESTORE_DEPTH_ACTION(MR_own_exits, (outer_count), (csn))

#define	MR_RESTORE_DEPTH_FAIL(outer_count, csn)				\
	MR_RESTORE_DEPTH_ACTION(MR_own_fails, (outer_count), (csn))

#define	MR_csn_vector_field(csn_vector, field_num)			\
	MR_field(MR_mktag(0), (csn_vector), (field_num))
::::::::::::::
mercury_deep_rec_depth_body.h
::::::::::::::
/*
** Copyright (C) 2001 The University of Melbourne.
** This file may only be copied under the terms of the GNU Library General
** Public License - see the file COPYING.LIB in the Mercury distribution.
*/

/*
** This macro implements the following predicates:
**	save_recursion_depth{1..9}
**	restore_recursion_depth_exit{1..9}
**	restore_recursion_depth_fail{1..9}
**
** The code including this file should define the following macros:
** 
** MR_PROCNAME:		The name of the procedure whose body this is.
** MR_REC_DEPTH_BODY:	A sequence of statements to execute in the body
** 			after setting up the following variables:
**
** 			pd: points to the ProcDynamic structure of the caller.
** 			ps: points to the ProcStatic structure of the caller.
**
** The code including this file should have the following variable in scope:
**
** CSD:			The id of the current csd.
*/

#ifdef MR_DEEP_PROFILING
{
  #ifdef MR_DEEP_PROFILING_TAIL_RECURSION
	MR_CallSiteDynamic	*csd;
	MR_ProcDynamic		*pd;
	MR_ProcStatic		*ps;
	MR_CallSiteDynamic	*inner_csd;
	int			inner_count;

	MR_enter_instrumentation();
	csd = (MR_CallSiteDynamic *) CSD;
	MR_deep_assert(csd == MR_current_call_site_dynamic);
	pd = csd->MR_csd_callee_ptr;
	MR_deep_assert(pd != NULL);
	ps = pd->MR_pd_proc_static;
	MR_deep_assert(ps != NULL);

	MR_REC_DEPTH_BODY

	MR_leave_instrumentation();
  #else
	MR_fatal_error(MR_PROCNAME ": tail recursion not enabled");
  #endif
}
#else
	MR_fatal_error(MR_PROCNAME ": deep profiling not enabled");
#endif

> I also want to see a relative diff for any changes that you make in
> response to these review comments!

Here it is. The change to mercury_exception_catch_body.h fixes a bug that
caused a test case failure in the debugging grades. Apart from that, and
duplicate_instance_2, my workspace gets no errors whatsoever in bootchecks
in asm_fast.gc, asm_fast.gc.debug.tr and asm_fast.gc.profdeep grades.

Zoltan.

Diffing browser
Diffing compiler
--- ws41.backup.may30//compiler/deep_profiling.m	Wed May 30 13:33:02 2001
+++ ws41/compiler/deep_profiling.m	Wed May 30 14:40:11 2001
@@ -1521,8 +1521,7 @@
 	VarTypes0 = DeepInfo0 ^ var_types,
 	VarName = string__format("CSN%d", [i(CSN)]),
 	varset__new_named_var(Vars0, VarName, CSNVar, Vars),
-	IntType = int_type,
-	map__set(VarTypes0, CSNVar, IntType, VarTypes),
+	map__set(VarTypes0, CSNVar, int_type, VarTypes),
 	DeepInfo = (DeepInfo0 ^ vars := Vars) ^ var_types := VarTypes,
 	generate_unify(int_const(CSN), CSNVar, UnifyGoal).
 
@@ -1576,8 +1575,7 @@
 generate_cell_unify(Length, ConsId, Args, Var, Goal) :-
 	Ground = ground(shared, none),
 	NonLocals = set__list_to_set([Var | Args]),
-	instmap_delta_from_assoc_list([Var - ground(shared, none)],
-		InstMapDelta),
+	instmap_delta_from_assoc_list([Var - Ground], InstMapDelta),
 	Determinism = det,
 	goal_info_init(NonLocals, InstMapDelta, Determinism, GoalInfo),
 	ArgMode = ((free - Ground) -> (Ground - Ground)),
Diffing deep_profiler
--- ws41.backup.may30//deep_profiler/mdprof_cgi.m	Wed May 30 13:36:06 2001
+++ ws41/deep_profiler/mdprof_cgi.m	Wed May 30 14:07:11 2001
@@ -40,13 +40,13 @@
 				process_query("menu", FileName)
 			;
 				io__write_string(
-					"Bad URL; expected query$:full:path:name")
+					"Bad URL; expected query$/full/path/name\n")
 			)
 		;
 			{ MaybeQueryString = no }
 		)
 	;
-		io__write_string("Usage: mdprof_cgi")
+		io__write_string("Usage: mdprof_cgi\n")
 	).
 
 :- pred process_query(string::in, string::in,
--- ws41.backup.may30//deep_profiler/timeout.m	Wed May 30 13:36:07 2001
+++ ws41/deep_profiler/timeout.m	Wed May 30 14:07:52 2001
@@ -44,7 +44,6 @@
 "
 #include <stdio.h>
 #include <signal.h>
-#include <unistd.h>
 #include ""mercury_signal.h""
 
 extern	char	*MP_timeout_file1;
@@ -63,21 +62,15 @@
 void
 delete_timeout_files_and_exit(void)
 {
-	char	buf[1000];
-
-	sprintf(buf, ""echo %s %s %s > /tmp/timeout"",
-		MP_timeout_file1, MP_timeout_file2, MP_timeout_file3);
-	system(buf);
-
-	if (unlink(MP_timeout_file1) != 0) {
+	if (remove(MP_timeout_file1) != 0) {
 		perror(MP_timeout_file1);
 	}
 
-	if (unlink(MP_timeout_file2) != 0) {
+	if (remove(MP_timeout_file2) != 0) {
 		perror(MP_timeout_file2);
 	}
 
-	if (unlink(MP_timeout_file3) != 0) {
+	if (remove(MP_timeout_file3) != 0) {
 		perror(MP_timeout_file3);
 	}
 
Diffing doc
Diffing library
Diffing runtime
--- ws41.backup.may30//runtime/mercury_deep_profiling.c	Wed May 30 13:34:43 2001
+++ ws41/runtime/mercury_deep_profiling.c	Wed May 30 14:38:40 2001
@@ -20,7 +20,7 @@
 #ifdef MR_DEEP_PROFILING
 
 #include <stdio.h>
-#include <unistd.h>
+#include <errno.h>
 
 MR_CallSiteStatic	MR_main_parent_call_site_statics[1] =
 {
@@ -197,7 +197,7 @@
 	kind_csd, kind_pd, kind_css, kind_ps
 } MR_NodeKind;
 
-/* must correspond to fixed_size_int_bytes in deep/read_profile.m */
+/* must correspond to fixed_size_int_bytes in deep_profiler/read_profile.m */
 #define	MR_FIXED_SIZE_INT_BYTES	4
 
 static	void	MR_write_csd_ptr(FILE *fp, const MR_CallSiteDynamic *ptr);
@@ -276,8 +276,8 @@
 
 	fp = fopen(MR_MDPROF_DATAFILENAME, "wb+");
 	if (fp == NULL) {
-		perror(MR_MDPROF_DATAFILENAME);
-		exit(1);
+		MR_fatal_error("cannot open `%s' for writing: %s",
+			MR_MDPROF_DATAFILENAME, strerror(errno));
 	}
 
 #ifdef	MR_DEEP_PROFILING_DEBUG
@@ -424,11 +424,7 @@
 static void
 MR_deep_data_output_error(const char *op)
 {
-	/* The op name and data file name are both fixed in length and short */
-	char	buf[1000];
-
-	sprintf(buf, "%s: %s", op, MR_MDPROF_DATAFILENAME);
-	perror(buf);
+	MR_warning("%s %s: %s", op, MR_MDPROF_DATAFILENAME, strerror(errno));
 
 	/*
 	** An incomplete profiling data file is useless. Removing it
@@ -437,9 +433,9 @@
 	** that.
 	*/
 
-	if (unlink(MR_MDPROF_DATAFILENAME) != 0) {
-		sprintf(buf, "%s: %s", "unlink", MR_MDPROF_DATAFILENAME);
-		perror(buf);
+	if (remove(MR_MDPROF_DATAFILENAME) != 0) {
+		MR_warning("cannot remove %s: %s",
+			MR_MDPROF_DATAFILENAME, strerror(errno));
 	}
 
 	exit(1);
@@ -448,7 +444,7 @@
 static void
 MR_write_out_id_string(FILE *fp)
 {
-	/* This string must match id_string deep/read_profile.m */
+	/* This string must match id_string deep_profiler/read_profile.m */
 	const char	*id_string = "Mercury deep profiler data";
 
 	fputs(id_string, fp);
--- ws41.backup.may30//runtime/mercury_exception_catch_body.h	Wed May 30 13:34:43 2001
+++ ws41/runtime/mercury_exception_catch_body.h	Wed May 30 14:03:56 2001
@@ -127,8 +127,8 @@
 	MR_deep_non_fail(proc_static, FIRST_DEEP_SLOT,
 		FAIL_PORT_RETURN_LABEL(proc_label));
 	/* non_redo_port_code executes *semidet* failure */
-	MR_fail();
 #endif
+	MR_fail();
 
 #endif	/* defined(version_model_non) && \
 		(defined(MR_USE_TRAIL) || defined(MR_DEEP_PROFILING)) */
Diffing scripts
Diffing trace
Diffing tools
Diffing util

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



More information about the reviews mailing list