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

[Ian MacLarty]

On Tue, 23 Aug 2005, Julien Fischer wrote:

>
> For review by anyone.
>
> Estimated hours taken: 3
> Branches: main, release
>
> Improvements to the deep profiling tool.
>
> Provide the facility to hide modules/procedures to which there
> have been no calls.  This reduces the amount of clutter
> when exploring the program module by module and when listing
> the procedures in a module (the clutter is primarily a result
> of unused standard library modules).  By default, we currently
> do not hide any modules/procedures.
>
> Add a toggle to sort lists of procedures by the number of redos
> they perform.  We have had control at the head of tables to do
> this for a while now.
>
> Delete the function int_list_from_to/2 since that functionality
> is now available in the standard library.
> 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.

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

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

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

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

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

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

Ian.

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