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

[Fergus Henderson]

On 17-May-2001, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:
> Index: compiler/layout_out.m
...
> +output_layout_name(proc_static(RttiProcLabel)) -->
> +	io__write_string(mercury_data_prefix),
> +	io__write_string("_proc_static__"),
> +	{ ProcLabel = code_util__make_proc_label_from_rtti(RttiProcLabel) },
> +	output_proc_label(ProcLabel).
> +	% io__write_string("_id").
> +	% { pred_id_to_int(RttiProcLabel ^ pred_id, PredId) },
> +	% io__write_int(PredId).

What are those commented out lines for?
Maybe they should be just deleted?

> +	io__write_string(",\n#ifdef MR_USE_ACTIVATION_COUNTS\n"),

That macro should be documented in runtime/mercury_conf_param.h.

> Index: compiler/options.m
> @@ -2064,6 +2093,9 @@
>  		"--memory-profiling\t\t(grade modifier: `.memprof')",
>  		"\tEnable memory and call profiling.",
>  		"\tThis option is not supported for the IL or Java back-ends.",
> +		"--deep-profiling\t\t(grade modifier: `.profdeep')",
> +		"\tEnable deep profiling.",
> +		"\tThis option is not supported for the HLC, IL or Java back-ends.",

I suggest s/HLC/high-level C/

> Index: compiler/unify_gen.m
> +unify_gen__generate_construction_2(
> +		deep_profiling_proc_static_tag(RttiProcLabel),
> +		Var, Args, _Modes, _, _, empty) -->
> +	( { Args = [] } ->
> +		[]
> +	;
> +		{ error("unify_gen: deep_profiling_proc_static has args") }
> +	),
> +	% code_info__get_module_info(ModuleInfo),
> +	% { PPId = proc(PredId, ProcId) },
> +	% { code_util__make_proc_label(ModuleInfo, PredId, ProcId, ProcLabel) },
> +	% { module_info_name(ModuleInfo, ModuleName) },
> +	{ DataAddr = layout_addr(proc_static(RttiProcLabel)) },
> +	code_info__assign_const_to_var(Var, const(data_addr_const(DataAddr))).

Why is the code there commented out?

> Index: deep/Mmakefile
...
> +$(cs_subdir)mdprof_cgi_init.c:	$(UTIL_DIR)/mkinit
> +$(cs_subdir)mdprof_server.c:	$(UTIL_DIR)/mkinit

I think the second line there is missing `_init' before the `.c'.

> +.PHONY: os cs
> +os: $(mdprof_cgi.os) $(os_subdir)mdprof_cgi_init.o
> +os: $(mdprof_server.os) $(os_subdir)mdprof_server.o
> +cs: $(mdprof_cgi.cs) $(cs_subdir)mdprof_cgi_init.c
> +cs: $(mdprof_server.cs) $(cs_subdir)mdprof_server.c

Likewise here (twice).

> Index: deep/conf.m
...
> +make_pipe_cmd(PipeName) = Cmd :-
> +	mkfifo_cmd(CmdName),
> +	(
> +		( string__remove_suffix(CmdName, "mkfifo", _)
> +		; string__remove_suffix(CmdName, "mkfifo_using_mknod", _)
> +		)
> +	->
> +		string__format("%s %s", [s(CmdName), s(PipeName)], Cmd)
> +	;
> +		error("make_pipe_cmd: do not know what command to use")
> +	).

I think it would be better here to test for CmdName \= ""
rather than hard-coding the requirement that it be *mkfifo
or *mkfifo_using_mknod.

> +++ dense_bitset.m	Mon May 14 12:51:48 2001
...
> +% This module provides an ADT for storing dense sets of small integers.
> +% The sets are represented as bit vectors, which are implemented as arrays
> +% of integers.

I think Ralph already commented that this was similar to
library/bitmap.m.  Could that be used instead?

> Index: deep/interface.m
> +to(Where, Cmd) -->
> +	io__tell(Where, Res),
> +	( { Res = ok } ->
> +		io__write(Cmd),
> +		io__write_string(".\n"),
> +		io__told
> +	;
> +		{ error("mdprof to: couldn't open pipe") }
> +	).

It's probably a good idea to use exceptions for errors here, but error/1
throws a software error exception, whereas this is really an I/O error.
So it would be better to use throw/1 rather than error/1.

(Likewise elsewhere in this file.)

> +++ mdprof_cgi.m	Thu May 17 01:36:05 2001
...
> +machine_name = "miles".

That hard-coding needs to be fixed.

> Index: deep/mdprof_server.m
...
> +:- func lookup_bool_option(option_table, option) = bool.
> +
> +lookup_bool_option(OptionTable, Option) = Value :-
> +	map__lookup(OptionTable, Option, TypedValue),
> +	( TypedValue = bool(ValuePrime) ->
> +		Value = ValuePrime
> +	;
> +		error("lookup_bool_option: option is not boolean")
> +	).

Why not just use getopt__lookup_bool_option?

> +:- func lookup_int_option(option_table, option) = int.
...
> +:- func lookup_string_option(option_table, option) = string.
...
> +:- func lookup_maybe_string_option(option_table, option) = maybe(string).

Likewise.

> Index: deep/read_profile.m
...
> +:- type ptr_info --->
> +		ptr_info(
> +			ps	:: int,
> +			css	:: int,
> +			pd	:: int,
> +			csd	:: int
> +		).
> +
> +:- type ptr_kind
> +	--->	ps
> +	;	pd
> +	;	css
> +	;	csd.

That's a bit cryptic; some comments would help.
Or you could use longer variable names.

> +read_profile(Res) -->
> +	read_num(Res0),
> +	(
> +		{ Res0 = ok(Mask) },
> +		{ MaybeError1 = no },
> +		% { MaybeError0 = no },
> +		% Calls are computed from the other counts below
> +		% ( { Mask /\ 0x0001 \= 0 } ->
> +		% 	maybe_read_num_handle_error(Calls,
> +		% 		MaybeError0, MaybeError1)
> +		% ;
> +		% 	{ Calls = 0 },
> +		% 	{ MaybeError1 = MaybeError0 }
> +		% ),
> +		( { Mask /\ 0x0002 \= 0 } ->
> +			maybe_read_num_handle_error(Exits,
> +				MaybeError1, MaybeError2)
> +		;
> +			{ Exits = 0 },
> +			{ MaybeError2 = MaybeError1 }
> +		),
> +		( { Mask /\ 0x0004 \= 0 } ->
> +			maybe_read_num_handle_error(Fails,
> +				MaybeError2, MaybeError3)
> +		;
> +			{ Fails = 0 },
> +			{ MaybeError3 = MaybeError2 }
> +		),
...

It looks like the file format is complicated.  It would be good to
either document the file format here, or if it is documented elsewhere,
to have a pointer here to where it is documented.

> Index: deep/server.m
> +test_server(DirName, Deep, Fields) -->
> +	{ string__format("mkdir -p %s", [s(DirName)], Cmd) },
> +	io__call_system(Cmd, _),

In the compiler directory we use

	{ string__format("[ -d %s ] || mkdir -p %s", [s(DirName)], Cmd) },

I think that some systems, mkdir will fail if the directory already exists.
It won't matter too much, since you ignore the result of mkdir anyway,
but you'd get a spurious error message.

Really we ought to have `mkdir' in the Mercury standard library.

> +:- func quantum_time(int) = string.
> +
> +quantum_time(Quanta) = TimeStr :-
> +	Time = Quanta * 10,	% a quantum is 10 milliseconds on our machines

Hmm, that looks like a non-portable assumption.

> +own_and_desc_to_html(Own, Desc, Deep, Fields) = HTML :-
> +	add_own_to_inherit(Own, Desc) = OwnPlusDesc,
> +	Root = root_total_info(Deep),
> +	Calls = calls(Own),
> +	Exits = exits(Own),
> +	Fails = fails(Own),
> +	Redos = redos(Own),
> +
> +	OwnQuanta = quanta(Own),
> +	TotalQuanta = inherit_quanta(OwnPlusDesc),
> +	RootQuanta = inherit_quanta(Root),
> +	OwnQuantaProp = 100.0 * float(OwnQuanta) / float(RootQuanta),
> +	TotalQuantaProp = 100.0 * float(TotalQuanta) / float(RootQuanta),
> +
> +	OwnAllocs = mallocs(Own),
> +	TotalAllocs = inherit_mallocs(OwnPlusDesc),
> +	RootAllocs = inherit_mallocs(Root),
> +	OwnAllocProp = 100.0 * float(OwnAllocs) / float(RootAllocs),
> +	TotalAllocProp = 100.0 * float(TotalAllocs) / float(RootAllocs),

What if RootAllocs = 0?
On some systems, you'll get a floating point exception.

Probably that should be written as something like

	TotalAllocProp = 100.0 * float(TotalAllocs)
			`checked_float_divide` float(RootAllocs),

where checked_float_divide is the same as ivide in profiler/generate_output.m.

Likewise for the other divisions here.

> Index: deep/startup.m
...
> +		error("emit nasal gorgons")

Cute ;-)

> +++ util.m	Mon May 14 12:51:51 2001
> @@ -0,0 +1,56 @@
> +:- interface.
> +
> +:- import_module char, list.
> +
> +:- pred split(string::in, char::in, list(string)::out) is det.

That semantics of that predicate should be documented.

> cvs diff: Diffing doc
> @@ -166,15 +166,17 @@
>  # Note that the private_builtin.m module is just an implementation
>  # detail of the library, so it is not documented.
>  
> -library-menu.texi: $(LIBRARY_DIR)/*.m
> +library-menu.texi: $(LIBRARY_DIR)/[a-z]*.m

What's the purpose of that change?
It doesn't seem to be described in the log message.

> Index: doc/user_guide.texi
...
>  @node Profiling introduction
> - at section Introduction
> + at section Profiling introduction
>  
> -The Mercury profiler @samp{mprof} is a tool which can be used to
> -analyze a Mercury program's performance, so that the programmer can
> -determine which predicates or functions are taking up a
> -disproportionate amount of the execution time.
> -
>  To obtain the best trade-off between productivity and efficiency,
>  programmers should not spend too much time optimizing their code
> -until they know which parts of the code are really taking up most
> -of the time.  Only once the code has been profiled should the
> -programmer consider making optimizations that would improve
> -efficiency at the expense of readability or ease of maintenance.
> -
> -A good profiler is a tool that should be part of every software
> -engineer's toolkit.
> +until they know which parts of the code are really taking up most of the time.
> +Only once the code has been profiled should the programmer consider
> +making optimizations that would improve efficiency
> +at the expense of readability or ease of maintenance.
> +A good profiler is therefore a tool
> +that should be part of every software engineer's toolkit.
> +
> +Mercury programs can be analyzed using two distinct profilers.
> +The Mercury profiler @samp{mprof} is a conventional graph profiler
> +in the style of gprof.

s/gprof/@samp{gprof}/

Also I suggest s/graph/call-graph/

> +Third, if enable graph profiling,
> +the compiler will generate for each source file
> +the static call graph for that file in @samp{@var{module}.prof}.

s/enable/you enable/

But this documentation is hard to understand, because the reader
may not know what graph profiling is, or what the purpose of these
.prof files is.

> + at item @code{--deep-profiling} (grades: any grade containing @samp{.profdeep})
> +Enable deep profiling by inserting the appropriate hooks in the generated code.
> +This option is not supported for the HLC, IL and Java back-ends.

I suggest s/HLC/high-level C/

> Index: library/Mmakefile
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/library/Mmakefile,v
> retrieving revision 1.66
> diff -u -b -r1.66 Mmakefile
> --- library/Mmakefile	2001/05/14 14:38:11	1.66
> +++ library/Mmakefile	2001/05/15 07:17:04
> @@ -80,7 +80,8 @@
>  			$(ENABLE_TERM_OPTS)
>  MCTOI	=	$(M_ENV) $(MC) --make-trans-opt $(INTERMODULE_OPTS) \
>  			$(ENABLE_TERM_OPTS)
> -MCG	=	$(M_ENV) $(MC) --compile-to-c --trace minimum \
> +# MCG	=	$(M_ENV) $(MC) --compile-to-c --trace minimum \
> +MCG	=	$(M_ENV) $(MC) --compile-to-c \
>  			$(INTERMODULE_OPTS) $(CHECK_TERM_OPTS)

Hmm, you commented out the `--trace minimum'.  Why? 

I don't think that bit should be committed.

> @@ -319,6 +320,11 @@
>  $(os_subdir)std_util.$O \
>  $(os_subdir)std_util.pic_o \
>  	: ../runtime/mercury_stack_layout.h
> +
> +# array.m contains C code that #includes exception.h. This requires us
> +# to force exception.h to be made before array.c is compiled.
> +$(os_subdir)array.$O: $(cs_subdir)exception.c
> +$(os_subdir)array.pic_o: $(cs_subdir)exception.c

That won't work with --target asm.

The dependency should be on the `.h' file, rather than on the `.c' file,
I think.  But I don't think that is enough to solve the problem in all
grades.

Won't the normal dependencies handle this correctly anyway?
The normal dependencies build all the .cs before building any of the .os.

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