[m-rev.] for review: mtc_union

[Julien Fischer]

On Fri, 5 Aug 2005, Ian MacLarty wrote:

> For review by anyone.
>
> Estimated hours taken: 10
> Branches: main
>
> Implement mtc_union program.  mtc_union takes a set of trace count files and
> combines them into one trace count file.  This is useful when slicing or dicing
> lots of files as it means all the files don't have to be individually loaded
> each time, which can take a long time.
>

I suggest adding a section to the user guide that describes all
the tools we have for manipulating trace count files, slices, dices
etc, as well as describing what they are and how they can be used.

> Mmakefile:
> 	Make mtc_union.
>
> compiler/tupling.m:
> 	Adjust for new trace_counts.m interface.
>
> mdbcomp/slice_and_dice.m:
> 	Do not return the individual trace counts file types when reading a
> 	slice.
> 	This information will not necessarily be available now, because
> 	the source may be a union.  Anyway this information is not used.
> 	The important bit of data, namely how many test cases were used
> 	to construct the slice, is still returned.
>
> 	Adjust for new trace_counts.m interface.
>
> mdbcomp/trace_counts.m:
> 	Introduce a new trace count file type: union(N), where
> 	N is how many test cases were used to construct the union.
>
> 	For the trace_counts type, store the filename with the proc id, instead
> 	of with each label.  The line numbers are still stored with each label.
> 	This makes it easier to write out the trace counts again and also
> 	probably uses less memory.
>
> 	Store the number of tests each label is executed in with each
> 	label.  This must be done to preserve this informationn after
s/informationn/information/

> 	trace counts have been unioned together.  The test case count
> 	may be omitted in the trace counts file, in which case it will be
> 	assumed to be one.
>
> 	Implement predicates to write out a trace count to a file.
>
> 	Add predicates to calculate the number of test cases used given
> 	a trace count file type.
>
> 	Modify read_trace_counts_list so that it merges the trace counts
> 	as it reads them.  This allows it to be used with long lists
> 	of trace count files without using excessive memory.
> 	Also optionally print out the name of each file read as it is
> 	read.
>
> slice/Mmakefile:
> 	Make mtc_union.
>
> slice/mtc_union.m:
> 	The mtc_union program.
>

...

> @@ -150,43 +185,44 @@
>      proc_trace_counts::out) is det.
>
>  sum_proc_trace_counts(ProcTraceCountsA, ProcTraceCountsB, ProcTraceCounts) :-
> -    ProcTraceCounts = map.union(sum_counts_in_context,
> +    ProcTraceCounts = map.union(sum_counts_on_line,
>          ProcTraceCountsA, ProcTraceCountsB).
>
> -:- func sum_counts_in_context(context_and_count, context_and_count)
> -    = context_and_count.
> +:- func sum_counts_on_line(line_no_and_count, line_no_and_count)
> +    = line_no_and_count.
>
> -sum_counts_in_context(CC1, CC2) = CC :-
> -    % We could add a sanity check that FileName1 = FileName2 and
> -    % LineNumber1 = LineNumber2, but that would take time to no good purpose,
> -    % since there isn't much we can do to generate a meaningful message,
> -    % and that situation doesn't necessarily represent an error anyway.
> -    % (Consider the case when the two trace files are derived from sources
> -    % that are identical except for the addition of a comment.)
> -
> -    CC1 = context_and_count(FileName1, LineNumber1, Count1),
> -    CC2 = context_and_count(_FileName2, _LineNumber, Count2),
> -    CC = context_and_count(FileName1, LineNumber1, Count1 + Count2).
> +    % We could add a sanity check that LineNumber1 = LineNumber2, but that
> +    % would take time to no good purpose, since there isn't much we can do to
> +    % generate a meaningful message, and that situation doesn't necessarily
> +    % represent an error anyway.  (Consider the case when the two trace files
> +    % are derived from sources that are identical except for the addition of a
> +    % comment.)
> +
That's not very clear - are you trying to explain why the "sanity check"
is in fact not a sanity check?

...

> -read_trace_counts_list_stream(MainFileName, Stream, Result, !IO) :-
> +read_trace_counts_list_stream(ShowProgress, FileType0, TraceCounts0,
> +        MainFileName, Stream, Result, !IO) :-
>      io.read_line_as_string(Stream, ReadResult, !IO),
>      (
>          ReadResult = ok(Line),
>          % Remove the trailing newline:
>          FileName = string.left(Line, string.length(Line) - 1),
> -        read_trace_counts(FileName, ReadTCResult, !IO),
>          (
> -            ReadTCResult = ok(FileType, TraceCount),
> -            read_trace_counts_list_stream(MainFileName, Stream, RestResult,
> -                !IO),
> -            ( RestResult = list_ok(FileTypesTraceCounts) ->
> -                Result = list_ok([FileType - TraceCount
> -                    | FileTypesTraceCounts])
> +            % Ignore blank lines.
> +            FileName = ""
> +        ->
> +            read_trace_counts_list_stream(ShowProgress, FileType0,
> +                TraceCounts0, MainFileName, Stream, Result, !IO)
> +        ;
> +            (
> +                ShowProgress = yes,
> +                io.write_string(FileName, !IO),
> +                io.nl(!IO)
> +            ;
> +                ShowProgress = no
> +            ),
> +            read_trace_counts(FileName, ReadTCResult, !IO),
> +            (
> +                ReadTCResult = ok(FileType1, TraceCounts1),
> +                summarize_trace_counts_list([TraceCounts0, TraceCounts1],
> +                    TraceCounts),
> +                NumTests = calc_num_tests([FileType0, FileType1]),
> +                FileType = union(NumTests),
> +                read_trace_counts_list_stream(ShowProgress, FileType,
> +                    TraceCounts, MainFileName, Stream, Result, !IO)
>              ;
> -                Result = RestResult
> +                ReadTCResult = io_error(IOError),
> +                Result = list_error_message("I/O error reading file " ++
> +                    "`" ++ FileName ++ "': " ++ string.string(IOError))
You might want to consider using io.error_message to get a better
description of the error (and below as well).

> +            ;
> +                ReadTCResult = open_error(IOError),
> +                Result = list_error_message("I/O error opening file " ++
> +                    "`" ++ FileName ++ "': " ++ string.string(IOError))
> +            ;
> +                ReadTCResult = syntax_error(Error),
> +                Result = list_error_message("Syntax error in file `" ++
> +                    FileName ++ "': " ++ Error)
> +            ;
> +                ReadTCResult = error_message(Message),
> +                Result = list_error_message("Error reading trace counts " ++
> +                    "from file `" ++ FileName ++ "': " ++ Message)
>              )

...

> @@ -525,16 +598,139 @@
>
>  %-----------------------------------------------------------------------------%
>
> +write_trace_counts_to_file(FileType, TraceCounts, FileName, Res, !IO) :-
> +    io__open_output(FileName, Result, !IO),
> +    (
> +        Result = ok(FileStream),
> +        Res = ok,
> +        io.set_output_stream(FileStream, OldOutputStream, !IO),
> +        io.write_string(trace_count_file_id, !IO),
> +        write_trace_counts(FileType, TraceCounts, !IO),
> +        io__set_output_stream(OldOutputStream, _, !IO),
> +        io__close_output(FileStream, !IO)

I suggest avoiding using different kinds of module qualifier in the
body of the same predicate.

...

>  #-----------------------------------------------------------------------------#
> Index: slice/mtc_union.m
> ===================================================================
> RCS file: slice/mtc_union.m
> diff -N slice/mtc_union.m
> --- /dev/null	1 Jan 1970 00:00:00 -0000
> +++ slice/mtc_union.m	4 Aug 2005 08:44:13 -0000
> @@ -0,0 +1,129 @@
> +%-----------------------------------------------------------------------------%
> +% Copyright (C) 2005 The University of Melbourne.
> +% This file may only be copied under the terms of the GNU General
> +% Public License - see the file COPYING in the Mercury distribution.
> +%-----------------------------------------------------------------------------%
> +%
> +% Tool to combine several trace counts into one.
> +%
Author?

> +%-----------------------------------------------------------------------------%
> +
> +:- module mtc_union.
> +
> +:- interface.
> +
> +:- import_module io.
> +
> +:- pred main(io::di, io::uo) is det.
> +

...

> +usage(!IO) :-
> +	io__write_strings([
> +		"Usage: mtc_union [-p] -o output_file file1 file2 ...\n",
> +		"The -p or --progress option causes each trace count ",
> +		"file name\n",
I would change the name of that option to '--verbose' so that it is
consistent with the other Mercury tools.  For the same reason consider
adding a '--help' option.

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