Re: [patch] Split Parse Timevar (issue4378056)

[Jason Merrill <jason@redhat.com>]

On 04/21/2011 07:17 PM, Lawrence Crowl wrote:
>>> @@ -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.

Sure, but the problem is that the collection doesn't always happen in 
the same place that generated most of the garbage.

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

I think we could describe both as language-specific finalization.

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

That makes sense.  Inlines in the class aren't significantly different 
from inlines outside the class, but inlines are significantly different 
from non-inlines for our purposes.

>>> -DEFTIMEVAR (TV_TEMPLATE_INSTANTIATION, "template instantiation")
>>> +DEFTIMEVAR (TV_INSTANTIATE_TEMPLATE  , "instantiate template")
>>
>> Why these changes?
>
> Just to shorten the names.

I'd prefer to keep it in the noun form.

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

And here you significantly lengthened one. :)

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

Hmm, I guess that makes sense, but it should be documented.  And perhaps 
move these timevars to the beginning or end so that they don't look like 
subsets of template instantiation.

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

The code is cleaner the way you have it, but not as correct, as there's 
some initialization being charged to parsing.

Jason