Re: [patch] Split Parse Timevar (issue4378056)

[Jason Merrill <jason@redhat.com>]

On 04/12/2011 11:49 AM, Lawrence Crowl wrote:
> This patch is available for review at http://codereview.appspot.com/4378056

I tried to comment there, but it didn't seem to be working; looking at 
the side-by-side diffs didn't show any changes, and double-clicking on a 
line in the patch form didn't let me add a comment.

> +  timevar_start (TV_RESOLVE_OVERLOAD);

Putting this in perform_overload_resolution isn't enough; only a couple 
of cases of overload resolution actually use it.  Any function that 
calls tourney will also need this.

> +lookup_template_class (tree d1, tree arglist, tree in_decl, tree context,
> +                      int entering_scope, tsubst_flags_t complain)
> +{
> +  tree ret;
> +  bool subtime = timevar_cond_start (TV_NAME_LOOKUP);

Let's count this as TV_INSTANTIATE_TEMPLATE instead.

> @@ -17194,7 +17225,7 @@ instantiate_decl (tree d, int defer_ok,
> -  timevar_push (TV_PARSE);
> +  timevar_push (TV_PARSE_GLOBAL);

This too.

> @@ -1911,7 +1911,7 @@ ggc_collect (void)
> -  timevar_push (TV_GC);
> +  timevar_start (TV_GC);

Why this change?  GC time shouldn't be counted against whatever we 
happen to be parsing when it happens.

> +DEFTIMEVAR (TV_PHASE_C_WRAPUP_CHECK  , "phase C wrapup & check")
> +DEFTIMEVAR (TV_PHASE_CP_DEFERRED     , "phase C++ deferred")

Why do these need to be different timevars?

> +DEFTIMEVAR (TV_PARSE_INMETH          , "parser inl. meth. body")

Is it really important to distinguish this from other functions?

> -DEFTIMEVAR (TV_NAME_LOOKUP           , "name lookup")
> -DEFTIMEVAR (TV_OVERLOAD              , "overload resolution")
> -DEFTIMEVAR (TV_TEMPLATE_INSTANTIATION, "template instantiation")
> +DEFTIMEVAR (TV_INSTANTIATE_TEMPLATE  , "instantiate template")
> +DEFTIMEVAR (TV_NAME_LOOKUP           , "|name lookup")
> +DEFTIMEVAR (TV_RESOLVE_OVERLOAD      , "|overload resolution")

Why these changes?

> @@ -564,6 +564,8 @@ compile_file (void)
> +  timevar_start (TV_PHASE_PARSING);

Why does this happen before...

> +  timevar_push (TV_PARSE_GLOBAL);

...this?  I would think the bits in there should be part of _SETUP.

> @@ -16760,6 +16770,7 @@ cp_parser_class_specifier (cp_parser* parser)
> +      timevar_pop (TV_PARSE_STRUCT);
> +      timevar_pop (TV_PARSE_STRUCT);
> +      timevar_pop (TV_PARSE_STRUCT);
> +  timevar_pop (TV_PARSE_STRUCT);

Why not factor this out like you did with so many functions outside the 
parser?

Jason