[m-rev.] for post-commit review: cords in the deep_profiler

Paul Bone pbone at csse.unimelb.edu.au
Tue Aug 5 21:18:16 AEST 2008


On Mon, Aug 05, 2008 at 01:16:40PM +1000, Zoltan Somogyi wrote:
> Convert Paul's new code to use cords of strings to represent HTML.
> 
> deep_profiler/html_format.m:
> 	Convert to using cords. Restructure the code in a couple of places
> 	to always put start and end tags around HTML fragments together.
> 
> 	Fix a missing "=" in a tag.
> 
> deep_profiler/interface.m:
> deep_profiler/mdprof_cgi.m:
> deep_profiler/read_profile.m:
> 	Provide better diagnostics.
> 
> deep_profiler/create_report.m:
> deep_profiler/display.m:
> deep_profiler/display_report.m:
> deep_profiler/mdprof_feedback.m:
> deep_profiler/measurement_units.m:
> deep_profiler/query.m:
> deep_profiler/report.m:
> mdbcomp/feedback.m:
> 	Misc cleanups. They can be considered my post-commit review of Paul's
> 	diff.
> 
> 	In mdprof_feedback.m, delete a strange test that prevented the program
> 	from being used from the command line.
> 
> deep_profiler/dump.m:
> deep_profiler/mdprof_dump.m:
> deep_profiler/timeout.m:
> deep_profiler/util.m:
> 	Misc cleanups of old code.
> 

Thanks for this Zoltan,  I have some minimal comments inline.

> 
> Index: deep_profiler/html_format.m
> ===================================================================
> RCS file: /home/mercury/mercury1/repository/mercury/deep_profiler/html_format.m,v
> retrieving revision 1.22
> diff -u -b -r1.22 html_format.m
> --- deep_profiler/html_format.m	29 Jul 2008 01:42:59 -0000	1.22
> +++ deep_profiler/html_format.m	3 Aug 2008 14:41:36 -0000
> @@ -11,15 +11,12 @@
>  %
>  % This module contains code that sets the format of the HTML tables
>  % we generate for individual queries.
> -%
> -% This module makes many calls to string.append.  In the C backends
> -% string.append is linear time over the length of both input strings when
> -% called in in, in, out mode.  If we build a long string from many short
> -% strings the cost will be quadratic.  It may be better to build a data
> -% structure with cords to describe how strings should be appended and then
> -% use calls to string.append_list at a final stage to construct a single
> -% string from the cord of strings.  There are alternative approaches that
> -% should reduce the final cost to linear.
> +
> +% This module appends many strings. Since string.append takes time that is
> +% linear over the length of both input strings, building a long string
> +% from many short strings would take quadratic time. This is why we represent
> +% HTML as a cord of strings instead. This cord is then converted to a list of
> +% strings and then a single list just before being given to the browser.
>  %
>  %-----------------------------------------------------------------------------%

Changes to this file look good.  I like the wrapping of HTML in tags.

>      
>      % Convert a display item into a HTML snippet.
>      %
> -:- pred item_to_html(http_context::in, display_item::in, string::out) is det.
> -
> -item_to_html(_, display_message(Message), HTML) :-
> -    HTML = "<h3>" ++ Message ++ "</h3>\n".
> -
> -item_to_html(HTTPContext, display_table(Table), HTML) :-
> -    table_to_html(HTTPContext, Table, HTML).
> +:- func item_to_html(string, string, http_context, display_item) = html.
>  
> -item_to_html(HTTPContext, display_list(Class, MaybeTitle, Items), HTML) :-
> -    some [!HTML]
> +item_to_html(StartTag, EndTag, HTTPContext, Item) = HTML :-
>      (
> +        Item = display_message(Message),
> +        HTML = wrap_tags(StartTag, EndTag,
> +            wrap_tags("<h3>", "</h3>\n", str_to_html(Message)))
> +    ;
> +        Item = display_command_link(DeepLink),
> +        HTML = wrap_tags(StartTag, EndTag,
> +            link_to_html(HTTPContext, DeepLink))
> +    ;
> +        Item = display_table(Table),
> +        HTML = wrap_tags(StartTag, EndTag,
> +            table_to_html(HTTPContext, Table))
> +    ;
> +        Item = display_list(Class, MaybeTitle, Items),
>          (
>              MaybeTitle = yes(Title),
> -            !:HTML = "<span id=""list_title"">" ++ Title ++ "</span>",
> +            TitleStartTag = "<span id=\"list_title\">",
> +            TitleEndTag = "</span>",
> +            TitleHTML = wrap_tags(TitleStartTag, TitleEndTag,
> +                str_to_html(Title)),
>              (
> -                Class = list_class_vertical_bullets
> +                Class = list_class_vertical_bullets,
> +                PostTitleHTML = empty_html
>              ;
>                  ( Class = list_class_horizontal
> -                ; Class = list_class_vertical_no_bullets ),
> -                !:HTML = !.HTML ++ "<br>"
> +                ; Class = list_class_vertical_no_bullets
> +                ),
> +                PostTitleHTML = str_to_html("<br>")
>              )
>          ;
>              MaybeTitle = no,
> -            !:HTML = ""
> -        ),
> -        (
> -            Class = list_class_vertical_bullets,
> -            Delim = "</li>\n<li>",
> -            !:HTML = !.HTML ++ "<ul><li>"
> -        ;
> -            Class = list_class_vertical_no_bullets,
> -            Delim = "<br>"
> -        ;
> -            Class = list_class_horizontal,
> -            Delim = " "
> +            TitleHTML = empty_html,
> +            PostTitleHTML = empty_html
>          ),
> -        map_join_html(Delim, item_to_html(HTTPContext), Items, HTML_Items),
> -        !:HTML = !.HTML ++ HTML_Items,
>          (
>              Class = list_class_vertical_bullets,
> -            HTML_End = "</li></ul>\n"
> +            OutsideStartTag = "<ul>",
> +            OutsideEndTag = "<ul>\n",

This end tag is incorrect,  it should be </ul>

> +            InnerStartTag = "<li>",         % XXX add \n?

I don't think we need a \n here, but it's good to put it after closing
tags.

> +            InnerEndTag = "</li>\n",
> +            Separator = empty_html
>          ;
>              Class = list_class_vertical_no_bullets,
> -            HTML_End = "<br>\n"
> +            OutsideStartTag = "",
> +            OutsideEndTag = "\n",
> +            InnerStartTag = "",
> +            InnerEndTag = "",               % XXX add \n?

I think a \n should be here, as it is a closing tag.  And below.

> +            Separator = str_to_html("<br>\n")
>          ;
>              Class = list_class_horizontal,
> -            HTML_End = "\n"
> -        ),
> -        !:HTML = !.HTML ++ HTML_End, 
> -        HTML = !.HTML
> +            OutsideStartTag = "",
> +            OutsideEndTag = "\n",
> +            InnerStartTag = "",
> +            InnerEndTag = "",
> +            Separator = str_to_html(" ")
> +        ),
> +        sep_map_join_html(Separator,
> +            item_to_html(InnerStartTag, InnerEndTag, HTTPContext), Items,
> +            InnerItemsHTML),
> +        ItemsHTML = wrap_tags(OutsideStartTag, OutsideEndTag, InnerItemsHTML),
> +        HTML = wrap_tags(StartTag, EndTag,
> +            TitleHTML ++ PostTitleHTML ++ ItemsHTML)
>      ).
>  
> -item_to_html(HTTPContext, display_command_link(DeepLink), HTML) :-
> -    link_to_html(HTTPContext, DeepLink, HTML).
> -
> -
> Index: deep_profiler/mdprof_feedback.m
> ===================================================================
> RCS file: /home/mercury/mercury1/repository/mercury/deep_profiler/mdprof_feedback.m,v
> retrieving revision 1.6
> diff -u -b -r1.6 mdprof_feedback.m
> --- deep_profiler/mdprof_feedback.m	30 Jul 2008 04:32:08 -0000	1.6
> +++ deep_profiler/mdprof_feedback.m	3 Aug 2008 14:16:34 -0000
> @@ -76,34 +77,37 @@
>          ->
>              write_help_message(ProgName, !IO)
>          ; 
> -            ProfileProgName \= "",
> -            Args = [Input, Output],
> +            % XXX What was the point of this test?
> +            % ProfileProgName \= "",
> +            Args = [InputFileName, OutputFileName],

This makes the -p argument composory, This is the name of the program
to which the profile belongs, this name is saved into the header of the
feedback file.  This way a user can identify which feedback files belong
to which programs.

This makes less sense once we support creating feedback from multiple
program runs of possibly different programs, in order to optimise a
libary.

>              check_options(Options, RequestedFeedbackInfo)
>          ->
>              lookup_bool_option(Options, verbose, Verbose),
> -            read_deep_file(Input, Verbose, MaybeDeep, !IO),
> +            read_deep_file(InputFileName, Verbose, MaybeDeep, !IO),
>              (
>                  MaybeDeep = ok(Deep),
> -                feedback.read_or_create(Output, Feedback0, !IO),
> +                feedback.read_or_create(OutputFileName, Feedback0, !IO),
>                  process_deep_to_feedback(RequestedFeedbackInfo, 
>                      Deep, Feedback0, Feedback),
> -                write_feedback_file(Output, ProfileProgName, Feedback,
> +                write_feedback_file(OutputFileName, ProfileProgName, Feedback,
>                      WriteResult, !IO),
>                  (
>                      WriteResult = ok
>                  ;
>                      ( WriteResult = open_error(Error)
> -                    ; WriteResult = write_error(Error) ),
> +                    ; WriteResult = write_error(Error)
> +                    ),
>                      io.error_message(Error, ErrorMessage),
> -                    io.format("%s: %s\n", [s(Output), s(ErrorMessage)], !IO),
> +                    io.format("%s: %s\n",
> +                        [s(OutputFileName), s(ErrorMessage)], !IO),
>                      io.set_exit_status(1, !IO)
>                  )
>              ;
>                  MaybeDeep = error(Error),
>                  io.stderr_stream(Stderr, !IO),
>                  io.set_exit_status(1, !IO),
> -                io.format(Stderr, "%s: error reading deep file: %s\n",
> -                    [s(ProgName), s(Error)], !IO)
> +                io.format(Stderr, "%s: error reading %s: %s\n",
> +                    [s(ProgName), s(InputFileName), s(Error)], !IO)
>              )   
>          ;
>              io.set_exit_status(1, !IO),


The rest of this diff is fine.


-------------- 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/20080805/b0a29dfd/attachment.sig>


More information about the reviews mailing list