[m-rev.] for post-commit review: Coverage Profiling completed.

[Paul Bone]

On Fri, Sep 26, 2008 at 05:06:19PM +1000, Zoltan Somogyi wrote:
> 
> Post-commit review of Paul's latest change to coverage profiling, together
> with some misc cleanups.
> 
> compiler/options.m:
> 	Add the new option --coverage-profiling-use-calls.
> 
> compiler/deep_profiling.m:
> 	Fix some misleading variable names and some misleading comments.
> 
> 	Make some comments more readable.
> 
> 	Group related predicates together.
> 
> 	Mark suspect code and unrevealing documentation with XXXs.
> 
> 	Fix an old bug: increment_coverage_point_counter does not bind its
> 	arguments.
> 
> 	Implement the new option.
> 
> library/profiling_builtin.m:
> 	Add a real body for increment_coverage_point_counter, since the new
> 	option needs it.
> 
> Index: compiler/deep_profiling.m
> ===================================================================
> RCS file: /home/mercury/mercury1/repository/mercury/compiler/deep_profiling.m,v
> retrieving revision 1.87
> diff -u -b -r1.87 deep_profiling.m
> --- compiler/deep_profiling.m	25 Sep 2008 13:44:48 -0000	1.87
> +++ compiler/deep_profiling.m	26 Sep 2008 06:53:39 -0000
> @@ -2236,6 +2252,7 @@
>      % Perform the coverage profiling transformation for conjuncts.  The goal
>      % list represents the list of goals within a conjunction minus 'Pos' goals
>      % removed from the head.
> +    % XXX What does that mean?
>      %
>  :- pred coverage_prof_second_pass_conj(conj_type::in,
>      list(hlds_goal)::in, list(hlds_goal)::out,

Fixed.

> @@ -2274,37 +2289,42 @@
>      proc_coverage_info::in, proc_coverage_info::out, bool::out) is det.
>  
>  coverage_prof_second_pass_disj(DPInfo, CoverageBeforeKnown,
> -        NextCoverageBeforeKnown, Disjs0, Disjs, !Info, AddedImpurity) :-
> +        NextCoverageBeforeKnown, Disjuncts0, Disjuncts, !Info,
> +        AddedImpurity) :-
>      % If the disjunction was introduced by the deep profiling pass, it has two
> -    % disjuncts and it's second disjunct has 'failure' determinism, then
> +    % disjuncts and its second disjunct has 'failure' determinism, then
>      % perform the coverage profiling pass on the first disjunct as if this
>      % is the only goal. 
>      (
>          DPInfo = dp_goal_info(goal_is_mdprof_inst, _),
> -        Disjs0 = [FirstDisj0, SecondDisj],
> -        SecondDisj ^ hlds_goal_info ^ goal_info_get_determinism = detism_failure
> +        Disjuncts0 = [FirstDisjunct0, SecondDisjunct],
> +        goal_info_get_determinism(SecondDisjunct ^ hlds_goal_info) =
> +            detism_failure
> +        % XXX Would this be a better test here?
> +        % goal_has_feature(SecondDisjunct, feature_preserve_backtrack_into)
>      ->
> -        coverage_prof_second_pass_goal(FirstDisj0, FirstDisj,
> +        coverage_prof_second_pass_goal(FirstDisjunct0, FirstDisjunct,
>              CoverageBeforeKnown, NextCoverageBeforeKnown, !Info,

I've added a comment here describing that the deep profiling
transformation doesn't appear to add this feature annotation to these
goals, but it probably should.

> @@ -2374,59 +2393,61 @@
>  
>  coverage_prof_second_pass_switchcase_2(_, _, [], [], _, _,
>          !CoverageAfterSwitchKnown, !Info, no). 
> -
>  coverage_prof_second_pass_switchcase_2(DPInfo, SwitchCanFail,
>          [Case0 | Cases0], [Case | Cases], CoverageBeforeSwitchKnown,
>          CoverageBeforeEveryCaseKnown, !CoverageAfterSwitchKnown, !Info,
>          AddedImpurity) :-
> -    Goal0 = Case0 ^ case_goal,
> +    Case0 = case(MainConsId, OtherConsIds, Goal0),
>  
> -    % If the switch cannot fail and this is the last case then the coverage
> -    % at the beginning of this case can be computed from the coverage before the
> -    % entire switch and coverage information for the tail of the switch such as
> -    % branch coverage points.
> +    % If the switch cannot fail and this is the last case, then the coverage
> +    % at the beginning of this case can be computed from the coverage before
> +    % the entire switch and coverage information for the tail of the switch
> +    % such as branch coverage points.
> +    % XXX The last part of that sentence does not make sense.
>      (
>          Cases0 = [],

Fixed.

> @@ -2778,18 +2753,19 @@
>      ;
>          GoalExpr0 = conj(ConjType, Goals0),
>          map_foldl2(coverage_prof_first_pass_conj(CPOptions), Goals0, Goals,
> -            goal_is_trivial, Trivial0, PortCountsCoverageAfterBefore,
> -            PortCountsCoverageAfterDirect),
> +            goal_is_trivial, Trivial0,
> +            PortCountsCoverageAfterBefore, PortCountsCoverageAfterDirect),
>          GoalExpr = conj(ConjType, Goals)
>      ;
>          GoalExpr0 = disj(Goals0),
> -        coverage_prof_first_pass_disj(CPOptions, Goals0, Goals,
> -            Trivial0, PortCountsCoverageAfterBefore,
> -            PortCountsCoverageAfterDirect0),
> +        coverage_prof_first_pass_disj(CPOptions, Goals0, Goals, Trivial0,
> +            PortCountsCoverageAfterBefore, PortCountsCoverageAfterDirect0),
>          GoalExpr = disj(Goals),
>  
>          % Only if the disjunction cannot fail can we know the coverage after
>          % the disjunction if port counts are available from every disjunct.
> +        % XXX What port counts? A disjunct need not contain a call.
> +        % do you mean coverage information from the end of each disjunct?
>          Detism = goal_info_get_determinism(GoalInfo0),
>          determinism_components(Detism, CanFail, _),
>          (

Not only is my earlier comment unclear, it's also false,  I've removed
it ad the code it refers to.

> @@ -2916,6 +2892,7 @@
>      !PortCountsCoverageAfter).
>  coverage_prof_first_pass_disj(CPOptions, [Goal0 | Goals0], [Goal | Goals],
>          Trivial, PortCountsCoverageAfterBefore, PortCountsCoverageAfter) :-
> +    % XXX What kind of name is PortCountsCoverageAfterBefore?
>      coverage_prof_first_pass(CPOptions, Goal0, Goal,
>          PortCountsCoverageAfterBefore,
>          dp_coverage_goal_info(TrivialGoal, PortCountsCoverageAfterGoal)),

Urgh,  Fixed.

In the future I'd like to add two extra types since I'm using
coverage_before_known in some places to express weather coverage is
known after a given goal, and port_counts_give_coverage_after to refer
to the coverage after a goal.


> @@ -2929,6 +2906,10 @@
>      % how often execution will enter the first case, so this also cannot use
>      % the portcount of a goal before it.
>      %
> +    % XXX This is wrong: there should be more differences than that, since you
> +    % a failure in one switch arm doesn't cause a jump to the start of the next
> +    % arm.
> +    %
>  :- pred coverage_prof_first_pass_switchcase(coverage_profiling_options::in,
>      list(case)::in, list(case)::out, goal_trivial::out,
>      port_counts_give_coverage_after::out) is det.

I've re-worded this.

> @@ -2977,14 +2977,26 @@
>          proc_static_cons_id(!.CoverageInfo, ProcStaticConsId),
>          generate_unify(ProcStaticConsId, ProcLayoutVar, GoalUnifyProcLayout),
>  
> -        !:CoverageInfo = !.CoverageInfo ^ ci_var_info := !.VarInfo
> +        !CoverageInfo ^ ci_var_info := !.VarInfo
>      ),
>  
>      % Build a call to the instrumentation code.
>  
> +    UseCalls = CPOptions ^ cpo_use_calls,
>      ModuleInfo = !.CoverageInfo ^ ci_module_info,
> -    get_deep_profile_builtin_ppid(ModuleInfo, "increment_coverage_point_count",
> -        2, PredId, ProcId),
> +    PredName = "increment_coverage_point_count",
> +    PredArity = 2,
> +    ArgVars = [ProcLayoutVar, CPIndexVar],
> +    % XXX The body of increment_coverage_point_count includes several
> +    % assertions. If these are enabled, then bodily including the C code
> +    % at EVERY coverage point will cause significant code bloat. Generating
> +    % a call to a predicate with the same code in library/profiling_builtin.m
> +    % should then yield smaller code, and due to cache effects, it will
> +    % probably yield faster code as well.
> +    (
> +        UseCalls = no,
> +        get_deep_profile_builtin_ppid(ModuleInfo, PredName, PredArity,
> +            PredId, ProcId),
>      Ground = ground(shared, none),
>      make_foreign_args([ProcLayoutVar, CPIndexVar],
>          [(yes("ProcLayout" - (Ground -> Ground)) - native_if_possible),

I've changed the "XXX" above to "Note", since this item doesn't require
any action, AIUI.

> @@ -3050,6 +3065,11 @@
>      some [!ForeignProcAttrs] (
>          % I don't think this would be thread safe but the cost of the mutexes
>          % may be too high.
> +        % XXX Correctness is more important than cost. The right solution
> +        % would be to add a mechanism that, in par grades, allows us to replace
> +        % the general foreign code mutex with one that guards only the data
> +        % coverage data structure we are updating, since that would yield
> +        % a LOT less contention.
>          !:ForeignProcAttrs = default_attributes(lang_c),
>          set_may_call_mercury(proc_will_not_call_mercury, !ForeignProcAttrs),
>          set_purity(purity_impure, !ForeignProcAttrs),

I agree, I think that what I meant when I wrote this was that you don't
want to pay for the cost of the mutexes in non parallel grades, (one of
Mercury's design principals), which I now understand is mostly what the
'par' grades enable.

I've added the 'not_thread_safe' attribute and modified this comment.

-------------- 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/20080927/a3d42db1/attachment.sig>