[m-rev.] Updated diff for deep profiling.
Fergus Henderson
fjh at cs.mu.OZ.AU
Tue May 29 18:15:51 AEST 2001
> +:- pred generate_single_csn_unify(int::in,
> + pair(prog_var, hlds_goal)::out, deep_info::in, deep_info::out) is det.
> +
> +generate_single_csn_unify(CSN, CSNVar - UnifyGoal, DeepInfo0, DeepInfo) :-
> + Vars0 = DeepInfo0 ^ vars,
> + 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),
Might as well just write
map__set(VarTypes0, CSNVar, int_type, VarTypes),
instead of the two lines above.
> +:- pred generate_cell_unify(int::in, cons_id::in, list(prog_var)::in,
> + prog_var::in, hlds_goal::out) is det.
> +
> +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),
The second occurrence of `ground(shared, none)' should be replaced by `Ground'.
> --- old/mercury/compiler/handle_options.m Tue May 29 16:07:51 2001
> +++ new/mercury/compiler/handle_options.m Tue May 29 16:13:25 2001
> @@ -523,8 +522,9 @@
>
> % The tail recursion optimization for deep profiling is implemented
> % only with --use-activation-counts.
> - 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.
> diff -u -b --recursive -x CVS old/mercury/deep_profiler/mdprof_cgi.m new/mercury/deep_profiler/mdprof_cgi.m
> --- old/mercury/deep_profiler/mdprof_cgi.m Tue May 29 16:07:51 2001
> +++ new/mercury/deep_profiler/mdprof_cgi.m Tue May 29 16:13:25 2001
> @@ -41,6 +44,9 @@
> )
> ;
> { MaybeQueryString = no }
> + )
> + ;
> + io__write_string("Usage: mdprof_cgi")
There should be a newline after the error message.
Also the message probably ought to go to stderr.
> diff -u -b --recursive -x CVS old/mercury/deep_profiler/timeout.m new/mercury/deep_profiler/timeout.m
> --- old/mercury/deep_profiler/timeout.m Tue May 29 16:07:52 2001
> +++ new/mercury/deep_profiler/timeout.m Tue May 29 16:13:25 2001
> @@ -51,10 +58,17 @@
> "
> char *MP_timeout_file1;
> char *MP_timeout_file2;
> +char *MP_timeout_file3;
>
> 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);
What's the purpose of that code?
I think there should at least be a comment there.
If this code was just for debugging, then it should be deleted.
The call to system() is not necessary here.
It would be more portable to use fopen() and fprintf().
> if (unlink(MP_timeout_file1) != 0) {
This should use the ANSI/ISO C remove() function
rather than the Posix unlink().
> diff -u -b --recursive -x CVS old/mercury/library/profiling_builtin.m new/mercury/library/profiling_builtin.m
> --- old/mercury/library/profiling_builtin.m Tue May 29 16:07:52 2001
> +++ new/mercury/library/profiling_builtin.m Tue May 29 16:13:26 2001
> @@ -112,15 +112,130 @@
>
> :- impure pred set_current_csd(call_site_dynamic::in) is det.
>
> -:- impure pred save_recursion_depth_count(call_site_dynamic::in,
> +:- type call_site_nums_2
> + ---> call_site_nums_2(int, int).
> +
> +:- type call_site_nums_3
> + ---> call_site_nums_3(int, int, int).
> +
> +:- type call_site_nums_4
> + ---> call_site_nums_4(int, int, int, int).
> +
> +:- type call_site_nums_5
> + ---> call_site_nums_5(int, int, int, int, int).
> +
> +:- type call_site_nums_6
> + ---> call_site_nums_6(int, int, int, int, int, int).
> +
> +:- type call_site_nums_7
> + ---> call_site_nums_7(int, int, int, int, int, int, int).
> +
> +:- type call_site_nums_8
> + ---> call_site_nums_8(int, int, int, int, int, int, int, int).
> +
> +:- type call_site_nums_9
> + ---> call_site_nums_9(int, int, int, int, int, int, int, int, int).
...
> +:- impure pred save_recursion_depth_1(call_site_dynamic::in,
> int::in, int::out) is det.
>
> -:- impure pred restore_recursion_depth_count_exit(
> +:- impure pred save_recursion_depth_2(call_site_dynamic::in,
> + call_site_nums_2::in, int::out, int::out) is det.
> +
> +:- impure pred save_recursion_depth_3(call_site_dynamic::in,
> + call_site_nums_3::in, int::out, int::out, int::out) is det.
> +
> +:- impure pred save_recursion_depth_4(call_site_dynamic::in,
> + call_site_nums_4::in, int::out, int::out, int::out, int::out) is det.
> +
> +:- impure pred save_recursion_depth_5(call_site_dynamic::in,
> + call_site_nums_5::in, int::out, int::out, int::out, int::out,
> + int::out) is det.
> +
> +:- impure pred save_recursion_depth_6(call_site_dynamic::in,
> + call_site_nums_6::in, int::out, int::out, int::out, int::out,
> + int::out, int::out) is det.
> +
> +:- impure pred save_recursion_depth_7(call_site_dynamic::in,
> + call_site_nums_7::in, int::out, int::out, int::out, int::out,
> + int::out, int::out, int::out) is det.
> +
> +:- impure pred save_recursion_depth_8(call_site_dynamic::in,
> + call_site_nums_8::in, int::out, int::out, int::out, int::out,
> + int::out, int::out, int::out, int::out) is det.
> +
> +:- impure pred save_recursion_depth_9(call_site_dynamic::in,
> + call_site_nums_9::in, int::out, int::out, int::out, int::out,
> + int::out, int::out, int::out, int::out, int::out) is det.
> +
> +:- impure pred restore_recursion_depth_exit_1(
> call_site_dynamic::in, int::in, int::in) is det.
>
> -:- impure pred restore_recursion_depth_count_fail(
> +:- impure pred restore_recursion_depth_exit_2(
> + call_site_dynamic::in, call_site_nums_2::in, int::in, int::in) is det.
> +
> +:- impure pred restore_recursion_depth_exit_3(
> + call_site_dynamic::in, call_site_nums_3::in, int::in, int::in,
> + int::in) is det.
> +
> +:- impure pred restore_recursion_depth_exit_4(
> + call_site_dynamic::in, call_site_nums_4::in, int::in, int::in,
> + int::in, int::in) is det.
> +
> +:- impure pred restore_recursion_depth_exit_5(
> + call_site_dynamic::in, call_site_nums_5::in, int::in, int::in,
> + int::in, int::in, int::in) is det.
> +
> +:- impure pred restore_recursion_depth_exit_6(
> + call_site_dynamic::in, call_site_nums_6::in, int::in, int::in,
> + int::in, int::in, int::in, int::in) is det.
> +
> +:- impure pred restore_recursion_depth_exit_7(
> + call_site_dynamic::in, call_site_nums_7::in, int::in, int::in,
> + int::in, int::in, int::in, int::in, int::in) is det.
> +
> +:- impure pred restore_recursion_depth_exit_8(
> + call_site_dynamic::in, call_site_nums_8::in, int::in, int::in,
> + int::in, int::in, int::in, int::in, int::in, int::in) is det.
> +
> +:- impure pred restore_recursion_depth_exit_9(
> + call_site_dynamic::in, call_site_nums_9::in, int::in, int::in,
> + int::in, int::in, int::in, int::in, int::in, int::in, int::in) is det.
> +
> +:- impure pred restore_recursion_depth_fail_1(
> call_site_dynamic::in, int::in, int::in) is det.
>
> +:- impure pred restore_recursion_depth_fail_2(
> + call_site_dynamic::in, call_site_nums_2::in, int::in, int::in) is det.
> +
> +:- impure pred restore_recursion_depth_fail_3(
> + call_site_dynamic::in, call_site_nums_3::in, int::in, int::in,
> + int::in) is det.
> +
> +:- impure pred restore_recursion_depth_fail_4(
> + call_site_dynamic::in, call_site_nums_4::in, int::in, int::in,
> + int::in, int::in) is det.
> +
> +:- impure pred restore_recursion_depth_fail_5(
> + call_site_dynamic::in, call_site_nums_5::in, int::in, int::in,
> + int::in, int::in, int::in) is det.
> +
> +:- impure pred restore_recursion_depth_fail_6(
> + call_site_dynamic::in, call_site_nums_6::in, int::in, int::in,
> + int::in, int::in, int::in, int::in) is det.
> +
> +:- impure pred restore_recursion_depth_fail_7(
> + call_site_dynamic::in, call_site_nums_7::in, int::in, int::in,
> + int::in, int::in, int::in, int::in, int::in) is det.
> +
> +:- impure pred restore_recursion_depth_fail_8(
> + call_site_dynamic::in, call_site_nums_8::in, int::in, int::in,
> + int::in, int::in, int::in, int::in, int::in, int::in) is det.
> +
> +:- impure pred restore_recursion_depth_fail_9(
> + call_site_dynamic::in, call_site_nums_9::in, int::in, int::in,
> + int::in, int::in, int::in, int::in, int::in, int::in, int::in) is det.
...
This is pretty ugly.
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?
> -:- pragma c_code(inner_call_port_code(ProcStatic::in, MiddleCSD::out),
> +:- pragma foreign_proc("C",
> + inner_call_port_code(ProcStatic::in, MiddleCSD::out),
> [thread_safe, will_not_call_mercury], "{
> #ifdef MR_DEEP_PROFILING
> - #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?
> +:- 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.
> +++ 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. Why is it needed?
Is it just for unlink()?
> @@ -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.)
> @@ -264,10 +271,12 @@
> MR_Proc_Id *pid;
> int root_pd_id;
> FILE *fp;
> + int ticks_per_sec;
>
> - fp = fopen("Deep.data", "w+");
> + fp = fopen(MR_MDPROF_DATAFILENAME, "wb+");
> if (fp == NULL) {
> - MR_fatal_error("Cannot open Deep.data");
> + perror(MR_MDPROF_DATAFILENAME);
> + exit(1);
You should MR_fatal_error() and strerror(),
so that all error messages emitted by the Mercury
runtime system are preceded by "Mercury runtime:",
rather than perror() followed by exit(1):
MR_fatal_error("cannot open `Deep.data' for writing: %s",
strerror(errno));
> 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);
You should use MR_warning() and strerror()
rather than sprintf() and perror().
> + /*
> + ** An incomplete profiling data file is useless. Removing it
> + ** prevents misunderstandings about that, and may also cure a
> + ** disk-full condition, if the close failure was caused by
> + ** that.
> + */
> +
> + if (unlink(MR_MDPROF_DATAFILENAME) != 0) {
> + sprintf(buf, "%s: %s", "unlink", MR_MDPROF_DATAFILENAME);
> + perror(buf);
> + }
That should use remove() rather than unlink().
> +static void
> MR_write_out_id_string(FILE *fp)
> {
> - /* 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/
----------
I think the code duplication in table_builtin.m should be avoided or at
least reduced. 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.
I also want to see a relative diff for any changes that you make in
response to these review comments!
--
Fergus Henderson <fjh at cs.mu.oz.au> | "I have always known that the pursuit
| 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
--------------------------------------------------------------------------
More information about the reviews
mailing list