[m-rev.] For review: clean up mdprof_dump command line parsing.

[Julien Fischer]

On Fri, 8 Feb 2008, Paul Bone wrote:

> Estimated hours taken: 2
>
> Branch: main
>
> Improved mdprof_dump's command line parsing.  It's now easier to predict
> mdprof_dump's behaviour.

It shouldn't be necessary to predict it all.  mdprof_dump's behaviour
should be sufficiently well documented that its behaviour should be
obvious to intended users, i.e. developers working on the deep
profiler.

> Major changes include The "-D restrict" option is
> now "--restrict", and is no-longer affected by other -D options.  Silently
> ignored -D options have now been documented as unimplemented.
>
> deep_profiler/dump.m:
> 	New types and predicates have been introduced to support a more
> 	strictly-checked set of options.  Including the dump_options type to
> 	represent options given to deep_dump/4 and deep_initial_dump/4.
> 	deep_dump/4, deep_initial_dump/4 and should_dump/2 have been modified to
> 	use the dump_options type.

I suggest:

 	Use a structured representation of the options used to control
 	what is printed by deep_dump/4 and deep_initial_dump/4.

> deep_profiler/startup.m:
> 	Conform to changes in dump.m, read_and_startup now takes a deep_options
> 	type rather than a list of strings as before.  A new version of

Delete "as before".

> 	read_and_startup has been created that doesn't take any deep_options and
> 	assumes the defaults.
>
> deep_profiler/mdprof_cgi.m:
> 	Conform to changes in startup.m, now makes use of deep.m.

s/deep.m/dump.m/
Merge this entry with the ones below.


> deep_profiler/mdprof_dump.m:
> 	Modified command line options, -D is only used to specify which arrays
> 	to dump and --restrict has been added which is no-longer affected by -D
> 	options.  Corrected usage message and conform to changes in dump.m.

I suggest:

 	The `-D' option is now only used to specify what arrays to
 	dump.  Add a separate option, --restrict, to control whether
 	unreferenced call_site_statics and proc_statics are printed.

>
> deep_profiler/mdprof_feedback.m:
> 	Conform to changes in startup.m, now makes use of deep.m.
>
> deep_profiler/mdprof_test.m:
> 	Conform to changes in startup.m, now makes use of deep.m.

You can merge some of those entries, e.g.

 	deep_profiler/mdprof_cgi.m:
 	deep_profiler/mdprof_feedback.m:
 	deep_profiler/mdprof_test.m:
 		Conform to the above changes.



> Index: deep_profiler/dump.m
> ===================================================================
> RCS file: /home/mercury/mercury1/repository/mercury/deep_profiler/dump.m,v
> retrieving revision 1.12
> diff -u -u -r1.12 dump.m
> --- deep_profiler/dump.m	23 Nov 2007 07:35:51 -0000	1.12
> +++ deep_profiler/dump.m	8 Feb 2008 11:55:19 -0000
> @@ -21,21 +21,106 @@
>
> :- import_module io.
> :- import_module list.
> +:- import_module set.
>
> %-----------------------------------------------------------------------------%
>
> +    % This type represtents the set of options that the dump_initial_deep
> +    % and dump_deep predicates below take.
> +    %

I suggest:

 	This structure controls what information is printed out by the
 	dump_initial_deep/4 and dump_deep/4 predicates.

> +:- type dump_options
> +    --->    dump_options(
> +                do_restricted               :: show_restricted_dump,
> +                do_arrays                   :: set(dumpable_array),
> +                do_stats                    :: show_stats,
> +                do_dump_cliques             :: dump_cliques,
> +                do_dump_rev_links           :: dump_rev_links,
> +                do_dump_prop_measurements   :: dump_prop_measurements
> +            ).
> +
> +
> +    % show_restricted_dump
> +    %


Delete the above two lines.

> +    % This type is used to describe if a restricted set of "css" and "ps"
> +    % structures should be shown, (those for code that was executed), or if all
> +    % "css" and "ps" structures should be shown.
> +    %
> +:- type show_restricted_dump
> +    --->        show_restricted_dump
> +    ;           show_complete_dump.
> +
> +
> +    % dumpable_array represents the availble arrays that may be selected
> +    % for dumping.
> +    %

I suggest:

 	This type indicates what type of arrays in the deep profile will
 	be printed.

> +:- type dumpable_array
> +    --->        csd
> +    ;           css
> +    ;           pd
> +    ;           ps.
> +
> +
> +    % show_stats describes whether to show some statistics and the root node
> +    % in the dump.
> +    %
> +:- type show_stats
> +    --->        show_stats
> +    ;           dont_show_stats.
> +
> +
> +    % Types to specifiy if cliques, rev (proc static to caller) links and
> +    % propogated measurements should be dumpped by dump_deep/3.

s/propogated/propagated/

> +    %
> +:- type dump_cliques
> +    --->        dump_cliques
> +    ;           dont_dump_cliques.

I prefer "do_not_", to "dont_".

> +
> +:- type dump_rev_links
> +    --->        dump_rev_links
> +    ;           dont_dump_rev_links.
> +
> +:- type dump_prop_measurements
> +    --->        dump_prop_measurements
> +    ;           dont_dump_prop_measurements.
> +

...

> +    % default_dump_options will retreive the default set of dump options.
> +    %

I suggest:

 	Returns the default set of dump options.


> +:- func default_dump_options = dump_options is det.

Functions in Mercury are det by default, so the determinism declaration
there is redundant.

> +    % dump_array_options will take a list of strings for the accumulating
> +    % dump options and produce a set if possible.
> +    %
> +    % A deterministic version is avalible that will throw an exception if
> +    % a string cannot be converted to a option.
> +    %
> +:- pred dump_array_options(list(string)::in, set(dumpable_array)::out)
> +    is semidet.
> +:- pred dump_array_options_det(list(string)::in, set(dumpable_array)::out)
> +    is det.

It would be more in keeping with the code in the rest of the system
to use `det_' as a prefix, rather than `_det' as a suffix.

> +    % dump_array_options_to_dump_options takes a list of strings of the
> +    % accumulating array dump options and create a dump_options structure
> +    % based on the default plus these spacific array options.
> +    %
> +:- pred dump_array_options_to_dump_options(list(string)::in,
> +    dump_options::out) is det.
> +
> +
>     % dump_initial_deep(InitialDeep, DumpOptions, !IO):
>     %
> -    % Dump selected parts of InitialDeep to standard output. The array of call
> -    % site dynamics, proc dynamics, call site statics and proc statics is
> -    % dumped if DumpOptions contains "csd", "pd", "css" or "ps" respectively.
> -    % If it contains "restrict", then the only the elements of the static
> -    % arrays that will be dumped are the ones that are referred to from the
> -    % dynamic arrays. The statistics and the root node are dumped if
> +    % Dump selected parts of InitialDeep to standard output. The array of
> +    % call site dynamics, proc dynamics, call site statics and proc statics
> +    % is dumped if DumpOptions contains "csd", "pd", "css" or "ps"
> +    % respectively.  The statistics and the root node are dumped if
>     % DumpOptions contains "stats".
>     %

I suggest:

 	Print selected parts of InitalDeep to the standard output.
 	Which parts are printed is controlled by DumpOptions.

> -:- pred dump_initial_deep(initial_deep::in, list(string)::in, io::di, io::uo)
> -    is det.
> +:- pred dump_initial_deep(initial_deep::in, dump_options::in, io::di,
> +    io::uo) is det.
>

...

> +default_dump_options = DumpOptions :-
> +    some [!ArraySet]
> +    (
> +        !:ArraySet = set.init,
> +        svset.insert(csd, !ArraySet),
> +        svset.insert(css, !ArraySet),
> +        svset.insert(pd, !ArraySet),
> +        svset.insert(ps, !ArraySet),
> +        DumpOptions = dump_options(show_complete_dump, !.ArraySet,
> +            show_stats, dump_cliques, dump_rev_links,
> +            dump_prop_measurements)
> +    ).

Why not just:

 	ArraySet = all_array_options

since you've already gone to the trouble of defining the function
all_array_options/0 below.

...


> +    % Handle special cases in the list of array options.
> +    %
> +:- pred dump_array_options_special(list(string)::in, set(dumpable_array)::out)
> +    is semidet.
> +
> +dump_array_options_special([], all_array_options).
> +dump_array_options_special(["all"], all_array_options).
> +
> +
> +dump_array_options_to_dump_options(Strings, DumpOptions) :-
> +    dump_array_options_det(Strings, DumpArrayOptions),
> +    DumpOptions = default_dump_options ^ do_arrays := DumpArrayOptions.
> +
> +
> +
> +:- pred string_list_to_sym_set(pred(string, X), list(string), set(X)).
> +:- mode string_list_to_sym_set(pred(in, out) is det, in, out) is det.
> +:- mode string_list_to_sym_set(pred(in, out) is semidet, in, out) is semidet.
> +
> +string_list_to_sym_set(StrToSym, List0, Set) :-
> +    map(StrToSym, List0, List),
> +    list_to_set(List, Set).
> +
> +
> +:- pred dump_array_option_det(string::in, dumpable_array::out) is det.
> +
> +dump_array_option_det(String, Array) :-
> +    ( dump_array_option(String, ArrayP) ->
> +        Array = ArrayP
> +    ;
> +        error("Invalid array name in dump options: " ++ String)
> +    ).
> +
> +
> +:- pred dump_array_option(string::in, dumpable_array::out) is semidet.

Rename this predicate string_to_dumpable_array/2.

> +dump_array_option("csd", csd).
> +dump_array_option("css", css).
> +dump_array_option("pd", pd).
> +dump_array_option("ps", ps).
> +
> +
> +:- func all_array_options = set(dumpable_array).
> +
> +all_array_options = Set :-
> +    some [!Set] (
> +        !:Set = set.init,
> +        svset.insert(csd, !Set),
> +        svset.insert(css, !Set),
> +        svset.insert(pd, !Set),
> +        svset.insert(ps, !Set),
> +        Set = !.Set
> +    ).

That can be written as:

 	all_array_options = set.from_list([csd, css, pd, ps]).

...

> Index: deep_profiler/mdprof_dump.m
> ===================================================================
> RCS file: /home/mercury/mercury1/repository/mercury/deep_profiler/mdprof_dump.m,v
> retrieving revision 1.7
> diff -u -u -r1.7 mdprof_dump.m
> --- deep_profiler/mdprof_dump.m	8 Nov 2006 05:24:37 -0000	1.7
> +++ deep_profiler/mdprof_dump.m	8 Feb 2008 11:55:19 -0000
> @@ -58,12 +58,21 @@
>         ;
>             NeedsHelp = no,
>             (
> -                Args = [],
> -                FileName = "Deep.data",
> -                main_2(Options, FileName, !IO)
> -            ;
> -                Args = [FileName],
> -                main_2(Options, FileName, !IO)
> +                (
> +                    Args = [],
> +                    FileName = "Deep.data"
> +                ;
> +                    Args = [FileName]
> +                ),
> +                % Process options, check that they make sense.
> +                make_dump_options(Options, MaybeDumpOptions),
> +                (
> +                    MaybeDumpOptions = yes(DumpOptions),
> +                    main_2(DumpOptions, FileName, !IO)
> +                ;
> +                    MaybeDumpOptions = no,
> +                    usage(ProgName, !IO)
> +                )
>             ;
>                 Args = [_, _ | _],
>                 usage(ProgName, !IO)
> @@ -76,10 +85,9 @@
>         io.set_exit_status(1, !IO)
>     ).
>
> -:- pred main_2(option_table(option)::in, string::in, io::di, io::uo) is det.
> +:- pred main_2(dump_options::in, string::in, io::di, io::uo) is det.
>
> -main_2(Options, FileName, !IO) :-
> -    getopt.lookup_accumulating_option(Options, dump_options, DumpOptions),
> +main_2(DumpOptions, FileName, !IO) :-
>     read_call_graph(FileName, MaybeInitialDeep, !IO),
>     (
>         MaybeInitialDeep = ok(InitialDeep),
> @@ -94,9 +102,36 @@
> % Option processing
> %
>
> +
> +    % Process options and the list of arrays to be dumped.
> +    %
> +:- pred make_dump_options(option_table(option)::in, maybe(dump_options)::out)
> +    is det.
> +
> +make_dump_options(Options, MaybeDumpOptions) :-
> +    getopt.lookup_accumulating_option(Options, dump_options, ArrayOptionStrs),
> +    getopt.lookup_bool_option(Options, option_restrict, RestrictBool),
> +    (
> +        RestrictBool = yes,
> +        Restrict = show_restricted_dump
> +    ;
> +        RestrictBool = no,
> +        Restrict = show_complete_dump
> +    ),
> +    DumpOptions0 = default_dump_options ^ do_restricted := Restrict,
> +    (
> +        dump_array_options(ArrayOptionStrs, ArrayOptions)
> +    ->
> +        MaybeDumpOptions = yes(DumpOptions0 ^ do_arrays := ArrayOptions)
> +    ;
> +        MaybeDumpOptions = no
> +    ).
> +
> +
> :- type option
>     --->    help
> -    ;       dump_options.
> +    ;       dump_options
> +    ;       option_restrict.
>
> :- type option_table == (option_table(option)).
>
> @@ -104,16 +139,19 @@
>
> short_option('h', help).
> short_option('D', dump_options).
> +short_option('r', option_restrict).
>
> :- pred long_option(string::in, option::out) is semidet.
>
> long_option("help", help).
> long_option("dump-options", dump_options).
> +long_option("restrict", option_restrict).
>
> :- pred defaults(option::out, option_data::out) is multi.
>
> defaults(help, bool(no)).
> defaults(dump_options, accumulating([])).
> +defaults(option_restrict, bool(no)).
>
> %----------------------------------------------------------------------------%
>
> @@ -131,6 +169,12 @@
>     "Options:\n" ++
>     "\t-h, --help\n" ++
>     "\t\tDisplay this message.\n" ++
> +    "\t-r, --restrict\n" ++
> +    "\t\tDo not dump proc and call-site statics that are\n" ++
> +    "\t\tnot referenced from the proc dynamics\n" ++
> +    "\t-D all\n" ++
> +    "\t\tAll of the following four options are implied,\n" ++
> +    "\t\t(the default if no -D options are given)\n" ++

That last sentence doesn't make sense as written.


>     "\t-D csd\n" ++
>     "\t\tDump call-site dynamics.\n" ++
>     "\t-D pd\n" ++
> @@ -139,9 +183,7 @@
>     "\t\tDump call-site statics.\n" ++
>     "\t-D ps\n" ++
>     "\t\tDump proc statics.\n" ++
> -    "\t-D restrict\n" ++
> -    "\t\tDo not dump proc and call-site statics that are\n" ++
> -    "\t\tnot referenced from the proc dynamics\n" ++
> +    "\nThese options arn't avalible in this version:\n" ++

s/avalible/available/, although I suggest:

 	The following options have not yet been implemented.

>     "\t-D clique\n" ++
>     "\t\tDump information about cliques.\n" ++
>     "\t-D rev\n" ++
> @@ -149,6 +191,7 @@
>     "\t-D prop\n" ++
>     "\t\tDump propagated measurement information.\n".

Julien.
--------------------------------------------------------------------------
mercury-reviews mailing list
Post messages to:       mercury-reviews at csse.unimelb.edu.au
Administrative Queries: owner-mercury-reviews at csse.unimelb.edu.au
Subscriptions:          mercury-reviews-request at csse.unimelb.edu.au
--------------------------------------------------------------------------