[m-rev.] for post-commit review: proc commands via reports

[Paul Bone]

On Mon, Aug 18, 2008 at 11:48:30AM +1000, Zoltan Somogyi wrote:
> For review by Paul. The diff for display_report.m is bigger than the file
> itself, so the file would be easier to review.
> 
> Zoltan.
> 
> Implement the proc command of the deep profiler using the report mechanism.
> 
> Some of the detailed changes also address Paul's post-commit review of my
> previous diff.
> 
> deep_profiler/report.m:
> 	Define the representation of the proc report.
> 
> 	Change some names to be more consistent.
> 
> deep_profiler/profile.m:
> 	Add a type for use in report.m.
> 
> 	Document the fields of an existing type.
> 
> deep_profiler/create_report.m:
> 	Add code to create proc reports.
> 
> 	Change some names to be more consistent.
> 
> 	Fix a bug: avoid divide by zero exceptions in some rare cases.
> 
> deep_profiler/measurement units.m:
> 	Fix a bug: avoid divide by zero exceptions in some rare cases.
> 
> deep_profiler/display_report.m:
> 	Add the code for converting proc reports to displays. This required
> 	rewriting almost everthing in this module to remove the assumption,
> 	which was previously embedded in many places, that the code is part
> 	of the implementation of the top_procs command. The new code is
> 	separated into two parts; the command-specific parts, and the parts
> 	that should be usable for all commands. (The reusable part is much
> 	bigger.)
> 
> deep_profiler/display.m:
> 	Expand the display representation to allow the specification of
> 
> 	- plain text as distinguished from headings
> 	- paragraph breaks
> 	- pseudo-links, i.e. text fragments that are formatted like links
> 	- table separator rows
> 	- table cells that span more than one column
> 
> 	which are used in the new version of display_report.m.
> 
> 	Simplify the mechanism for setting up table header groups.
> 
> deep_profiler/html_format.m:
> 	Handle the changes in display.m.
> 
> 	Fix some anomalies in the formatting of lists and tables.
> 
> 	Group related code together.
> 
> deep_profiler/query.m:
> 	Provide a switch for selecting which of the two mechanisms,
> 	the old direct generation of HTML or the new indirect generation
> 	through the report and display structures, should generate the HTML
> 	page for a command that both mechanisms can implement. The default
> 	is the new mechanism, but it can be overridden by implementors
> 	by creating a file called "/tmp/old_deep_profiler".
> 
> 	Switch the default handling of the proc command over to the new
> 	mechanism.
> 
> 	Rename some types and function symbols to make them more consistent
> 	and expressive.
> 
> 	Generate more informative error messages for domain error exceptions.
> 
> deep_profiler/mdprof_cgi.m:
> 	Add a mechanism for decoding the links in the generated pages.
> 	The mechanism takes the form of three new options: --decode,
> 	--decode-cmd, and --decode-prefs. When one or more of these are given,
> 	mdprof_cgi will not do its usual stuff, instead of just loops waiting
> 	for lines whose contents it will then try to decode as queries,
> 	as commands and/or as preferences.
> 
> deep_profiler/Mercury.options:
> 	Try to postpone thrashing in the deep compiler when compiled in
> 	debug grades, by compiling their contents with shallow tracing.
> 
> deep_profiler/array_util.m:
> 	Make shallow tracing effective by moving the deep recursions
> 	into non-exported predicates.
> 
> deep_profiler/callgraph.m:
> deep_profiler/canonical.m:
> deep_profiler/startup.m:
> 	Convert the debugging code to use trace goals.
> 
> deep_profiler/cliques.m:
> deep_profiler/read_profile.m:
> 	Convert the debugging code to use trace goals.
> 
> 	Use more expressive names for lots of variables.
> 
> 	In read_profile.m, avoid an unnecessary repackaging of data.
> 
> deep_profiler/util.m:
> 	Fix some grammar errors in a comment, and comment some code to use
> 	state variables.
> 
> deep_profiler/interface.m:
> 	Fix some grammar errors in a comment.
> 
> deep_profiler/mdprof_feedback.m:
> 	Change some variable names to be consistent with the other modules.
> 
> library/float.m:
> library/int.m:
> 	Make the messages in the domain errors we throw more expressive.
> 
> library/io.m:
> 	Minor style changes.

deep_profiler/create_report.m 

    in int_per_call uses has an if-then-else as an expression rather
    than a goal.  I've seen this before in Mercury programs, but havn't
    seen it documented as an expression.  Is this in the language
    reference manual?

deep_profiler/display_report.m

    A ZZZ style comment has been left in on line 1310.

deep_profiler/display.m

> Index: display.m
> ===================================================================
> RCS file: /home/mercury/mercury1/repository/mercury/deep_profiler/display.m,v
> retrieving revision 1.6
> retrieving revision 1.7
> diff -u -p -r1.6 -r1.7
> --- display.m	6 Aug 2008 08:15:03 -0000	1.6
> +++ display.m	18 Aug 2008 02:14:51 -0000	1.7
> @@ -20,7 +20,6 @@
>  :- import_module measurement_units.
>  :- import_module query.
>  
> -:- import_module cord.
>  :- import_module list.
>  :- import_module maybe.
>  :- import_module string.
> @@ -34,8 +33,24 @@
>              ).
>  
>  :- type display_item
> -    --->    display_message(string)
> -    ;       display_table(table)
> +    --->    display_heading(
> +                % A string to be displayed as a HTML header.
> +                string

This comment shouldn't refer to HTML, this datastructure represents an
abstract display of deep profiling data.

> +            )
> +    ;       display_text(
> +                % A string to be displayed as ordinary text.
> +                string
> +            )
> +    ;       display_paragraph_break
> +    ;       display_link(
> +                deep_link
> +            )
> +    ;       display_pseudo_link(
> +                % A string to be formatted exactly as if it were a link,
> +                % without it actually being a link. Used for situations
> +                % when a link would lead back to this page.
> +                pseudo_link
> +            )

I suggest.  s/this page/the same page/

> @@ -87,7 +103,7 @@
>                  % about how to paint this column.
>                  thg_class       :: table_column_class,
>  
> -                thg_set_style   :: table_set_style
> +                thg_colour      :: table_column_colour
>              ).
>  
>  :- type table_header_group_columns

It should be noted that this colour refers to the 'shading' of every
second column group.

deep_profiler/query.m:
    
    In exec, the variable named FileExists should be named
    UseOldCGICode, and could perhaps have a type of it's own.

    A ZZZ style comment has been left in this predicate.

deep_profiler/util.m:
    
    The split predicates here can probably be replaced by, or defined in
    terms of the split predicates in string.m.

The rest is fine.

Thanks.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mercurylang.org/archives/reviews/attachments/20080820/0a9bd2bc/attachment.sig>