Re: [patch] Split Parse Timevar (issue4378056)

[Lawrence Crowl <crowl@google.com>]

On 4/20/11, Jason Merrill <jason@redhat.com> wrote:
> On 04/12/2011 11:49 AM, Lawrence Crowl wrote:
>> This patch is available for review at
>> http://codereview.appspot.com/4378056
>> +  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.

Okay, I'll add those functions.  You've explained why I was getting
surprisingly low numbers for overload resolution.

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

Okay.  BTW, that is a change from existing behavior.

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

Okay.

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

If not, then code that generates lots of garbage does not get
charged for the cost to collect it.  I thought it best to separate
these issues.

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

The are measuring different things.  They are less different now
than they were during earlier development.  We can make them the
same if you want.

>> +DEFTIMEVAR (TV_PARSE_INMETH          , "parser inl. meth. body")
>
> Is it really important to distinguish this from other functions?

This distinction is here to help evaluate potential speedup due to
lazy parsing.  It might make some sense to separate functions and
inline functions, which also wouldn't have to be parsed immediately.

>> -DEFTIMEVAR (TV_TEMPLATE_INSTANTIATION, "template instantiation")
>> +DEFTIMEVAR (TV_INSTANTIATE_TEMPLATE  , "instantiate template")
>
> Why these changes?

Just to shorten the names.  They were getting pretty unwieldy.

>> -DEFTIMEVAR (TV_NAME_LOOKUP           , "name lookup")
>> -DEFTIMEVAR (TV_OVERLOAD              , "overload resolution")
>> +DEFTIMEVAR (TV_NAME_LOOKUP           , "|name lookup")
>> +DEFTIMEVAR (TV_RESOLVE_OVERLOAD      , "|overload resolution")
>
> Why these changes?

The "|" (also in TV_GC) indicates that these vars are collecting
time concurrently with the other non-phase variables.  It is intended
to remind readers not to add those times into totals.

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

We could do that, though it would involve splitting the start/stop
calls into different functions.  That seemed hard to manage.
As it stands, TV_PHASE_SETUP is entirely before compile_file()
and TV_PHASE_FINALIZE is entirely after.  Thoughts?

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

Probably just inertia early on.  I'll change it.

-- 
Lawrence Crowl