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

[Zoltan Somogyi]

On 18-May-2001, Fergus Henderson <fjh at cs.mu.OZ.AU> wrote:
> What are those commented out lines for?
> Maybe they should be just deleted?

Done.

> > +	io__write_string(",\n#ifdef MR_USE_ACTIVATION_COUNTS\n"),
> 
> That macro should be documented in runtime/mercury_conf_param.h.

Done.

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

Done.

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

Deleted.

> I think the second line there is missing `_init' before the `.c'.
> Likewise here (twice).

Fixed.

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

Done.

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

You are asking the wrong person. Tom wrote that, not me. However, I believe
he wrote it before Ralph wrote, or at least committed, bitmap.m.

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

Well, the system design tests for the existence of those pipes, and creates
them if they do not initially exist, so if they cannot be opened, it is either
a design error or a sign of intervention by God (i.e. root).

> > +++ mdprof_cgi.m	Thu May 17 01:36:05 2001
> ...
> > +machine_name = "miles".
> 
> That hard-coding needs to be fixed.

I have already removed all calls to the predicate, but initially forgot to
delete the predicate itself; I have now done so.

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

Good idea. Done.

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

These abbreviations are pervasive in the deep profiler paper; I have documented
them in the file I have added on the design of the deep profiler.

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

Its structure is outlined in the file on the deep profiler's design.

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

I will do that, but I will use test instead of [. I think we should avoid that
particular obsolete algol68-lookalike hack in sh.

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

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

Printing zero would be misleading. I have added a new function for returning a
printable percentage, and made it return "N/A" when the divisor is zero.

> > +:- pred split(string::in, char::in, list(string)::out) is det.
> 
> That semantics of that predicate should be documented.

Done.

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

I can undo it if you like, but it allows you to make backup copies of library
modules in files starting with an upper case letter, without that action
causing a time-consuming rebuilding of the library manual.

> s/gprof/@samp{gprof}/
> Also I suggest s/graph/call-graph/
> s/enable/you enable/
> I suggest s/HLC/high-level C/

All done.

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

I have undone that; it was for debugging only. I wanted deep tracing of
library code.

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

There are no rules for making the .h file, therefore one can't put a
dependency on it. Its builting is a side-effect, and takes place only
if the module exports some procedures.

> Won't the normal dependencies handle this correctly anyway?

No. I added the rule after finding that out.

> The normal dependencies build all the .cs before building any of the .os.

That depends (depended) on RM_C.

However, I have made the reporting of array bound errors via exception
an implementor only facility, controlled by the ML_ARRAY_THROW_EXCEPTIONS.
Since implementors can manually make exception.h if required, I have deleted
the dependency of array.o on exception.c.
--------------------------------------------------------------------------
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
--------------------------------------------------------------------------