[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