[m-rev.] for review: improvements to deep profiler

[Julien Fischer]

On Wed, 24 Aug 2005, Ian MacLarty wrote:

> > Index: html_format.m
> > ===================================================================
> >  default_fields = fields(port, ticks, no_alloc, memory(words)).
> > @@ -306,6 +320,7 @@
> >  default_scope = overall.
> >  default_contour = no_contour.
> >  default_time_format = scale_by_thousands.
> > +default_inactive_items = inactive_items(show, show).
> >
>
> Does this mean the default is to show inactive procs and modules?  If so I
> would suggest that the default be to hide them.
>
In the long run we should do that, for now I'd prefer this way.

> > +
> > +:- func inactive_items_to_string(inactive_items) = string.
> > +
> > +inactive_items_to_string(inactive_items(hide, hide)) = "hh".
> > +inactive_items_to_string(inactive_items(show, hide)) = "sh".
> > +inactive_items_to_string(inactive_items(hide, show)) = "hs".
> > +inactive_items_to_string(inactive_items(show, show)) = "ss".
> > +
> > +:- pred string_to_inactive_items(string::in, inactive_items::out) is semidet.
> > +
> > +string_to_inactive_items("hh", inactive_items(hide,  hide)).
> > +string_to_inactive_items("sh", inactive_items(show,  hide)).
> > +string_to_inactive_items("hs", inactive_items(hide,  show)).
> > +string_to_inactive_items("ss", inactive_items(show,  show)).
> >
>
> Making the above one predicate with two modes would be better.
>
It is convenient to have a function version.

> > Index: query.m
> > ===================================================================
> >  	ModuleName \= "Mercury runtime".
> >
> >  :- func module_summary_to_html(preferences, deep, pair(string, module_data))
> > -	= one_id_line.
> > +	= one_id_line is semidet.
> >
>
> I suggest changing this function to a predicate if that's not too
> inconvenient.
>
Leaving at a function means that less code elsewhere needs to be
modified.  Also, I prefer using the function version of list.filter_map
which is what this is used with.

> >  module_summary_to_html(Pref, Deep, ModuleName - ModuleData) = LineGroup :-
> > -	Own = ModuleData ^ module_own,
> > -	Desc = ModuleData ^ module_desc,
> > -	HTML =
> > -		string__format("<TD><A HREF=""%s"">%s</A></TD>\n",
> > -			[s(deep_cmd_pref_to_url(Pref, Deep,
> > -				module(ModuleName))),
> > -			s(ModuleName)]),
> > -	LineGroup = line_group(ModuleName, 0, ModuleName, Own, Desc, HTML,
> > -		unit).
> > +	ModuleData = module_data(Own, Desc, _),
> > +	( Pref ^ pref_inactive = inactive_items(_, hide), is_inactive(Own) ->
> > +		fail
> > +	;
> > +		HTML =
> > +			string__format("<TD><A HREF=""%s"">%s</A></TD>\n",
> > +				[s(deep_cmd_pref_to_url(Pref, Deep,
> > +					module(ModuleName))),
> > +				s(ModuleName)]),
> > +		LineGroup = line_group(ModuleName, 0, ModuleName, Own, Desc,
> > +			HTML, unit)
> > +	).
>
> Put the conjuncts in the if then else condition on seperate lines.
>
> I think putting the following instead of the if then else would read better:
>
> not (
> 	Pref ^ pref_inactive ^ inactive_modules = hide,
> 	is_inactive(Own)
> ),
> HTML = ...
>
Done.

> >  %-----------------------------------------------------------------------------%
> >
> > +	% Fails if the procedure is inactive and the preferences say to
> > +	% hide inactive procedures.
> > +	%
> >  :- func lookup_proc_total_to_html(preferences, deep, bool, string,
> > -	proc_static_ptr) = one_id_line.
> > +	proc_static_ptr) = one_id_line is semidet.
> >
>
> I suggest changing this function to a predicate if that's not too
> inconvenient.
>
Not done, for the same reasons as above.

> >  lookup_proc_total_to_html(Pref, Deep, Bold, Prefix, PSPtr) = LineGroup :-
> >  	deep_lookup_ps_own(Deep, PSPtr, Own),
> > -	deep_lookup_ps_desc(Deep, PSPtr, Desc),
> > -	LineGroup = proc_total_to_html(Pref, Deep, Bold, Prefix,
> > -		PSPtr, Own, Desc).
> > +	(
> > +		Pref ^ pref_inactive = inactive_items(hide, _),
> > +		is_inactive(Own)
> > +	->
> > +		fail
>
> I suggest putting the condition in a not and doing away with the
> if then else.  Also using the field access function on pref_inactive
> makes it clearer which field should have the value `hide'.
>
Done.

> > +	;
> > +		deep_lookup_ps_desc(Deep, PSPtr, Desc),
> > +		LineGroup = proc_total_to_html(Pref, Deep, Bold, Prefix,
> > +			PSPtr, Own, Desc)
> > +	).
> >
> >  :- func lookup_proc_total_to_two_id_line(preferences, deep, bool, string,
> >  	proc_static_ptr) = two_id_line.
>
> Otherwise the diff looks cool.
>

Thanks for that.

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