Re: [patch] Split Parse Timevar (issue4378056)

[Lawrence Crowl <crowl@google.com>]

On 4/21/11, Jason Merrill <jason@redhat.com> wrote:
> 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.

True, but I expect it usually does.  At any rate, I will revert
the timevar to push/pop.

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

Okay.

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

Do you have a quick hint for how to make that distinction?

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

Okay.  This on in particular was making the output wide.

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

Ah, but it wasn't the long pole and hence more clarity didn't hurt.

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

Okay.

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

Would you prefer moving that initialization out or placing the
start/stop into different routines?

-- 
Lawrence Crowl