On 4/21/11, Jason Merrill <ja...@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
