[m-rev.] For post-commit review: Coverage profiler no-longer instruments deep profiler instrumentation goals.

[Zoltan Somogyi]

On 14-Sep-2008, Paul Bone <pbone at csse.unimelb.edu.au> wrote:
> +        % XXX: This computation seems broken, it's been disabled so I can
> +        % relyably implement coverage profiling.  Test it on
> +        % erlang_rtti_implementation.deconstruct_2/9-2 whose switch's type has
> +        % 25 constructors yet this computes 27.  Are constructors different to
> +        % functors?

I visually checked erlang_rtti_implementation.deconstruct_2, and the switch
variable in it DOES have 27 constructors; the switch covers them all.
The switch has 25 arms, because the last arm is shared by three function
symbols.

What problem did you think you were fixing by disabling this code?

> @@ -1106,7 +1115,8 @@ deep_prof_wrap_call(GoalPath, Goal0, Goa
>      ModuleInfo = !.DeepInfo ^ deep_module_info,
>      GoalFeatures = goal_info_get_features(GoalInfo0),
>      goal_info_remove_feature(feature_tailcall, GoalInfo0, GoalInfo1),
> -    make_impure(GoalInfo1, GoalInfo),
> +    make_impure(GoalInfo1, GoalInfoImpure),
> +    goal_info_set_mdprof_inst(goal_is_mdprof_inst, GoalInfoImpure, GoalInfo),

I think the name "GoalInfo" like this is misleading (by not saying enough).
I am fixing this.

> @@ -2035,102 +2068,111 @@ coverage_prof_second_pass_goal(Goal0, Go
>      % Consider inserting a coverage point after this goal to measure how
>      % many solutions it may have.
>      (
> -        GoalTrivial = goal_is_nontrivial,
> +        IsMDProfInst = goal_is_not_mdprof_inst,
>          (
> -            GoalHasPortCounts = goal_does_not_have_port_counts,
> +            GoalTrivial = goal_is_nontrivial,
>              (
> -                CoverageAfterKnown0 = coverage_after_unknown,
> -
> +                GoalHasPortCounts = goal_does_not_have_port_counts,
>                  (

All this is too deeply indented to be easily readable, and unnecessarily so,
since all you are doing is checking for a series of conditions that make a
coverage point unnecessary. I have fixed this.

In general, you should send diffs with diff's -b option enabled, as this
avoids showing places where nothing changed except the indentation.

> -                ThenMaybeDPInfo = 
> -                    goal_info_get_maybe_dp_info(Then0 ^ hlds_goal_info),
> +                goal_info_get_maybe_dp_coverage_info(Then0 ^ hlds_goal_info) =
> +                    ThenMaybeCoverageInfo,
>                  (
> -                    ThenMaybeDPInfo = yes(dp_goal_info(_, ThenHasPortCounts)) 
> +                    ThenMaybeCoverageInfo = yes(dp_coverage_goal_info(_,
> +                        ThenHasPortCounts)) 
>                  ;
> -                    ThenMaybeDPInfo = no,
> +                    ThenMaybeCoverageInfo = no,
>                      ThenHasPortCounts = goal_does_not_have_port_counts
>                  ),

This looks like repeated code, so I have factored it out.

> @@ -2522,17 +2557,21 @@ coverage_prof_second_pass_ite(ITEExistVa
>      % beginning of each branch,
>  
>      CPOBranchIf = !.Info ^ ci_coverage_profiling_opts ^ cpo_branch_ite,
> -    CondMaybeDPInfo = goal_info_get_maybe_dp_info(Cond0 ^ hlds_goal_info),
> +    CondMaybeDPCoverageInfo = goal_info_get_maybe_dp_coverage_info(
> +        Cond0 ^ hlds_goal_info),
>      (
> -        CondMaybeDPInfo = yes(dp_goal_info(_, CondHasPortCounts))
> +        CondMaybeDPCoverageInfo = 
> +            yes(dp_coverage_goal_info(_, CondHasPortCounts))
>      ;
> -        CondMaybeDPInfo = no,
> +        CondMaybeDPCoverageInfo = no,
>          CondHasPortCounts = goal_does_not_have_port_counts
>      ),
> +    DPInfo = dp_goal_info(IsMDProfInst, _),
>      (
>          CPOBranchIf = yes,
>          CondHasPortCounts = goal_does_not_have_port_counts,
> -
> +        IsMDProfInst = goal_is_mdprof_inst
> +    ->

This test looks like it should be reversed. I have marked it with an XXX.

> +        % XXX: Not all call goals have associated call sites, therefore not all
> +        % of these will have port counts.  See above in the deep_profiling
> +        % transformation.
> +        %
> +        % This doesn't matter for the near future, since we're using a single
> +        % pass coverage profiling algorithm.  This will be important when a
> +        % naive assumption in the second pass is fixed (even when this first
> +        % pass is not used).
> +        %
 
I don't understand this comment, so I marked it with another XXX.

>      ( string.contains_char(Verbose, 'E') ->
>          MaybeDPInfo = goal_info_get_maybe_dp_info(GoalInfo),
>          (
> -            MaybeDPInfo = yes(dp_goal_info(Trivial, HasPortCounts)),
> -            write_indent(Indent, !IO),
> +            MaybeDPInfo = yes(dp_goal_info(_, MaybeDPCoverageInfo)),

You don't print out the instrumentation flag field; I fixed that.

> +                MaybeDPCoverageInfo = yes(dp_coverage_goal_info(Trivial,
> +                    HasPortCounts)),
> +                write_indent(Indent, !IO),
> +                (
> +                    Trivial = goal_is_trivial,
> +                    io.write_string("% Goal is trivial, ", !IO)
> +                ;
> +                    Trivial = goal_is_nontrivial,
> +                    io.write_string("% Goal is non-trivial, ", !IO)
> +                ),
> +                (
> +                    HasPortCounts = goal_has_port_counts,
> +                    io.write_string("Goal has port counts avalible.\n", !IO)
> +                ;
> +                    HasPortCounts = goal_does_not_have_port_counts,
> +                    io.write_string("Goal does not have port counts avalible.\n", 
> +                        !IO)

Apart from the misspellings, the output lines generated by this code
are likely to be too long; I have fixed it.

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