Re: [PATCH] Move global state in timevar.c to a new "timer" class

[Jeff Law <law at redhat dot com>]

On 05/08/2015 03:41 PM, David Malcolm wrote:
> I want to make the timevar infrastructure be more flexible,
> to better serve some JIT use-cases.
>
> The following is a "removal of global state" patch which gathers
> together the state within timevar.c into a new "timer" class.
>
> Within the classic cc1/cc1plus/etc use-case, this timer class
> is a singleton.  For the JIT, I have a followup patch allowing
> timers to be managed externally by client code, so that client code
> can e.g. have multiple compiles in-process, and accumulate the
> timings into different timers as appropriate (so e.g. a JavaScript
> engine on one thread could accumulate its JIT-compilation timings
> into one timer, whereas a Python JIT on another thread could
> accumulate into another timer, and various other arrangements).
>
> There is a global singleton for gcc's internals:
>
>    timer *g_timer;
>
> (which is guarded by the jit mutex for the jit case).
>
> Currently the global bool "timevar_enable" is used for early bailout
> from the timevar code, for the common needs-to-be-fast case where
> timevar-tracking is disabled.  The patch eliminates this flag
> throughout, replacing it with a test for g_timer being non-NULL
> (it is only non-NULL if timing has been enabled).
>
> The existing API implicitly uses "g_timer", so e.g.
>
>    timevar_push (TV_SOMETHING);
>
> which was:
>
>    if (timevar_enable)
>      timevar_push_1 (TV_SOMETHING);
>
> effectively becomes:
>
>    if (g_timer)
>      g_timer->push (TV_SOMETHING);
>
> A detail relating to timevar.c's get_time: "g_timer" is a singleton
> as far as most of gcc is concerned, but I have a follow patch in
> development in which there could be multiple timer instances in the
> jit.  Hence the patch eliminated a check for timevar_enable in
> "get_time" , *without* replacing it with a check for
> g_timer != NULL, since with this patch get_time is only called from
> methods of timer.  Doing so enables us to call methods on other timer
> instances (e.g. to measure time spent waiting for the jit mutex).
> There's also a test eliminated from timer::print, for similar reasons.
>
> With timing disabled, this patch replaces the early reject
> using a global bool with an early reject using a global ptr, and so
> ought to be as fast:
>
> test-benchmark.c, with timing disabled (the default case):
>   Before introducing "class timer":
>    100 iterations at optlevel 0 took a total of 6.150s (0.061s per iteration)
>    100 iterations at optlevel 1 took a total of 6.780s (0.068s per iteration)
>    100 iterations at optlevel 2 took a total of 6.990s (0.070s per iteration)
>    100 iterations at optlevel 3 took a total of 8.590s (0.086s per iteration)
>   After introducing "class timer:
>    100 iterations at optlevel 0 took a total of 6.120s (0.061s per iteration)
>    100 iterations at optlevel 1 took a total of 6.840s (0.068s per iteration)
>    100 iterations at optlevel 2 took a total of 7.020s (0.070s per iteration)
>    100 iterations at optlevel 3 took a total of 8.570s (0.086s per iteration)
>
> With timing enabled, there's an extra indirection inside the
> code in timevar.c, but this appears to have no significant cost,
> for the jit.df/test-benchmark.c at least (though this test has just one
> function):
Yea, I wouldn't expect it to impact things at any measurable level.

> test-benchmark.c, with timing enabled:
>   Before introducing "class timer":
>    100 iterations at optlevel 0 took a total of 6.110s (0.061s per iteration)
>    100 iterations at optlevel 1 took a total of 6.970s (0.070s per iteration)
>    100 iterations at optlevel 2 took a total of 7.080s (0.071s per iteration)
>    100 iterations at optlevel 3 took a total of 8.820s (0.088s per iteration)
>   After introducing "class timer":
>    100 iterations at optlevel 0 took a total of 6.170s (0.062s per iteration)
>    100 iterations at optlevel 1 took a total of 6.980s (0.070s per iteration)
>    100 iterations at optlevel 2 took a total of 7.040s (0.070s per iteration)
>    100 iterations at optlevel 3 took a total of 8.730s (0.087s per iteration)
>
> The patch moves the structs "timevar_def" and "timevar_stack_def"
> from timevar.c into timevar.h, hiding them as private within
> class timer.  I had another version of this patch which kept them
> within timevar.c, but to do this I had to use the "pimpl" idiom,
> adding complexity and another layer of indirection.  It seemed best
> to simply put them as private types within class timer.
>
> Successfully bootstrapped&regrtested on x86_64-unknown-linux-gnu
> (Fedora 20).
>
> OK for trunk?
>
> gcc/ChangeLog:
> 	* timevar.c (timevar_enable): Delete in favor of...
> 	(g_timer): New global.
> 	(struct timevar_def): Move to timevar.h inside class timer.
> 	(struct timevar_stack_def): Likewise.
> 	(timevars): Delete global in favor of field "m_timevars" within
> 	class timer in timevar.h
> 	(stack): Likewise, in favor of field "m_stack".
> 	(unused_stack_instances): Likewise, in favor of field
> 	"m_unused_stack_instances".
> 	(start_time): Likewise, in favor of field "m_start_time".
> 	(get_time): Eliminate check for timevar_enable.
> 	(timer::timer): New function, built from part of timevar_init.
> 	(timevar_init): Rewrite idempotency test from using
> 	"timevar_enable" bool to using dynamic allocation of "g_timer".
> 	Move rest of implementation into timer's constructor.
> 	(timevar_push_1): Rename to...
> 	(timer::push): ...this, adding "m_" prefixes to variables that
> 	are now fields of timer.
> 	(timevar_pop_1): Likewise, rename to...
> 	(timer::pop): ...this, and add "m_" prefixes.
> 	(timevar_start): Replace test for "timevar_enable" with one for
> 	"g_timer", and move bulk of implementation to...
> 	(timer::start): ...here, adding "m_" prefixes.
> 	(timevar_stop): Likewise, from here...
> 	(timer::stop): ...to here.
> 	(timevar_cond_start): Likewise, from here...
> 	(timer::cond_start): ...to here.
> 	(timevar_cond_stop): Likewise, from here...
> 	(timer::cond_stop): ...to here.
> 	(validate_phases): Rename to...
> 	(timer::validate_phases): ...this, and add "m_" prefixes.  Make
> 	locals "total" and "tv" const.
> 	(timevar_print): Rename to...
> 	(timer::print): ...this, and add "m_" prefixes.  Make locals
> 	"total" and "tv" const.  Eliminate test for timevar_enable.
> 	* timevar.h (timevar_enable): Eliminate.
> 	(g_timer): New declaration.
> 	(timevar_push_1): Eliminate.
> 	(timevar_pop_1): Eliminate.
> 	(timevar_print): Eliminate.
> 	(class timer): New class.
> 	(timevar_push): Rewrite to use g_timer.
> 	(timevar_pop): Likewise.
> 	* toplev.c (toplev::~toplev): Likewise.
> ---
OK.
jeff