Re: [patch] Split Parse Timevar (issue4378056)

[Lawrence Crowl]

This discussion continues in the thread
[patch] Split Parse Timevar (rev 2) (issue4433076)
which has a new uploaded patch.

On 4/23/11, Jason Merrill ja...@redhat.com wrote:
 On 04/22/2011 06:41 PM, Lawrence Crowl wrote:
 On 4/21/11, Jason Merrillja...@redhat.com  wrote:
 On 04/21/2011 07:17 PM, Lawrence Crowl wrote:

 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?

 Check DECL_DECLARED_INLINE_P after we've parsed the declarator.

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

 I wouldn't mind shortening it to TV_TEMPLATE_INST, I just object to the
 change from noun (instantiation) to verb (instantiate).

 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?

 I think moving the initialization out would be better.

 Jason



-- 
Lawrence Crowl

Re: [patch] Split Parse Timevar (issue4378056)

[Jason Merrill]

On 04/22/2011 06:41 PM, Lawrence Crowl wrote:

On 4/21/11, Jason Merrillja...@redhat.com  wrote:

On 04/21/2011 07:17 PM, Lawrence Crowl wrote:



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?


Check DECL_DECLARED_INLINE_P after we've parsed the declarator.


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


I wouldn't mind shortening it to TV_TEMPLATE_INST, I just object to the 
change from noun (instantiation) to verb (instantiate).



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?


I think moving the initialization out would be better.

Jason

Re: [patch] Split Parse Timevar (issue4378056)

[Lawrence Crowl]

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

Re: [patch] Split Parse Timevar (issue4378056)

[Diego Novillo]

On Wed, Apr 20, 2011 at 18:00, Jason Merrill ja...@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

 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.

If you are logged in and click on the file (not the side-by-side), you
can double-click on the patch hunk you want to comment on (you'll need
to cut-n-paste some context due to
http://code.google.com/p/rietveld/issues/detail?id=291can=4).

Now, for some reason I never could quite grasp, some of Lawrence's
patches were uploaded in such a way that both the base file and the
patched file are the same.  That's why the side-by-side view is not
working.

Subsequent patches from Lawrence did not have that problem.  So,
whatever it was, it got fixed (but I'm not sure what fixed it).


Diego.

Re: [patch] Split Parse Timevar (issue4378056)

[Jason Merrill]

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

Re: [patch] Split Parse Timevar (issue4378056)

[Jason Merrill]

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

Re: [patch] Split Parse Timevar (issue4378056)

[Diego Novillo]

On Tue, Apr 12, 2011 at 14:49, Lawrence Crowl cr...@google.com wrote:
 This patch provides more finer, more precise compile time information.
 I sent an advisory mail some time ago, and it was good then.  Please
 confirm for trunk.

The patch looks fine to me, but of course it's Jason the one you need
an OK from.



Diego.