Subzero: Add a few performance measurement tools.

Subzero: Add a few performance measurement tools.

--timing-funcs - Produces a sorted list of total time spent translating each function.

--timing-focus=<F> - Turns on the --timing equivalent just for one function.  Use '*' to do this for all functions, i.e. get complete timing breakdowns across all functions.

--verbose-focus=<F> - Temporarily turns on --verbose=all for one function.

BUG= none
R=jvoung@chromium.org

Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=8363a0668ce21ea0c2a61f78083b0536dbd89860

[Jim Stichnoth, 2014-10-04 00:02:06]

stichnot@chromium.org changed reviewers:
+ jvoung@chromium.org, kschimpf@google.com

[Jim Stichnoth, 2014-10-04 00:15:24]

https://codereview.chromium.org/620373004/diff/1/src/IceCfgNode.cpp
File src/IceCfgNode.cpp (left):

https://codereview.chromium.org/620373004/diff/1/src/IceCfgNode.cpp#oldcode498
src/IceCfgNode.cpp:498: if (First)
Whoops, there were 3 cases like this where I got the Auto-Awesome logic
backwards.

[jvoung (off chromium), 2014-10-06 16:30:56]

https://codereview.chromium.org/620373004/diff/20001/src/IceCfg.cpp
File src/IceCfg.cpp (right):

https://codereview.chromium.org/620373004/diff/20001/src/IceCfg.cpp#newcode76
src/IceCfg.cpp:76: bool VerboseFocus = (getContext()->getFlags().VerboseFocusOn
== getFunctionName());
80 col ?

https://codereview.chromium.org/620373004/diff/20001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/620373004/diff/20001/src/IceGlobalContext.cpp...
src/IceGlobalContext.cpp:416: Timers[StackID].dump(getStrDump(),
DumpCumulative);
Maybe the dump() can indicate which StackID the dump is for? Right now it should
be obvious which stack it's for though, based on the timer names (pass names vs
function names).

https://codereview.chromium.org/620373004/diff/20001/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/620373004/diff/20001/src/IceGlobalContext.h#n...
src/IceGlobalContext.h:164: static TimerStackIdT getTimerStackID();
How is getTimerStackID() planned to be used? (I don't see a user in this patch
set)

At first I thought there was a new timerstackid per function, but it looks like
it's a separate timer ID instead (using the function name).

https://codereview.chromium.org/620373004/diff/20001/src/IceTimerTree.h
File src/IceTimerTree.h (right):

https://codereview.chromium.org/620373004/diff/20001/src/IceTimerTree.h#newco...
src/IceTimerTree.h:41: // TimerStack(const TimerStack &) = delete;
Is this because of std::vector?

[Jim Stichnoth, 2014-10-06 21:29:57]

Along with the code review suggestions, I got rid of the misguided non-POD
static initialization pattern I was using for timer stacks and timer IDs.  They
are now part of the GlobalContext object which essentially keeps them
singletons, but without the potentially weird side effects of static
initializers.

https://codereview.chromium.org/620373004/diff/20001/src/IceCfg.cpp
File src/IceCfg.cpp (right):

https://codereview.chromium.org/620373004/diff/20001/src/IceCfg.cpp#newcode76
src/IceCfg.cpp:76: bool VerboseFocus = (getContext()->getFlags().VerboseFocusOn
== getFunctionName());
On 2014/10/06 16:30:56, jvoung wrote:
> 80 col ?

Done.

https://codereview.chromium.org/620373004/diff/20001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/620373004/diff/20001/src/IceGlobalContext.cpp...
src/IceGlobalContext.cpp:416: Timers[StackID].dump(getStrDump(),
DumpCumulative);
On 2014/10/06 16:30:56, jvoung wrote:
> Maybe the dump() can indicate which StackID the dump is for? Right now it
should
> be obvious which stack it's for though, based on the timer names (pass names
vs
> function names).

Done.

https://codereview.chromium.org/620373004/diff/20001/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/620373004/diff/20001/src/IceGlobalContext.h#n...
src/IceGlobalContext.h:164: static TimerStackIdT getTimerStackID();
On 2014/10/06 16:30:56, jvoung wrote:
> How is getTimerStackID() planned to be used? (I don't see a user in this patch
> set)
> 
> At first I thought there was a new timerstackid per function, but it looks
like
> it's a separate timer ID instead (using the function name).

The thought is that some component, e.g. the bitcode parser, could use this
along with instrumenting its own code.  The advantage is that it's easily
accessible through the GlobalContext object.

BTW, I changed this to be non-static, and the GlobalContext ctor uses it to
pre-allocate the two predefined timer stacks.

https://codereview.chromium.org/620373004/diff/20001/src/IceTimerTree.h
File src/IceTimerTree.h (right):

https://codereview.chromium.org/620373004/diff/20001/src/IceTimerTree.h#newco...
src/IceTimerTree.h:41: // TimerStack(const TimerStack &) = delete;
On 2014/10/06 16:30:56, jvoung wrote:
> Is this because of std::vector?

Yes, specifically the Timers.push_back() call.

[jvoung (off chromium), 2014-10-06 23:01:52]

https://codereview.chromium.org/620373004/diff/40001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/620373004/diff/40001/src/IceGlobalContext.cpp...
src/IceGlobalContext.cpp:396: return NumTimerStackIds++;
Could this just use Timers.size() (or return void)?

https://codereview.chromium.org/620373004/diff/40001/src/IceTimerTree.cpp
File src/IceTimerTree.cpp (right):

https://codereview.chromium.org/620373004/diff/40001/src/IceTimerTree.cpp#new...
src/IceTimerTree.cpp:41: for (auto I = IDs.rbegin(), E = IDs.rend(); I != E;
++I, --Result) {
I wonder if this should be a map, or unorder_map, now that the main use now is
for functions (there can be a lot for a linear search)?

On the other hand, there shouldn't be a hit for functions the way that
getTimerID() is used, except if it has internal linkage?

[jvoung (off chromium), 2014-10-06 23:02:36]

https://codereview.chromium.org/620373004/diff/40001/src/IceTimerTree.cpp
File src/IceTimerTree.cpp (right):

https://codereview.chromium.org/620373004/diff/40001/src/IceTimerTree.cpp#new...
src/IceTimerTree.cpp:41: for (auto I = IDs.rbegin(), E = IDs.rend(); I != E;
++I, --Result) {
On 2014/10/06 23:01:52, jvoung wrote:
> I wonder if this should be a map, or unorder_map, now that the main use now is
> for functions (there can be a lot for a linear search)?
> 
> On the other hand, there shouldn't be a hit for functions the way that
> getTimerID() is used, except if it has internal linkage?

Err... except for the PNaClTranslator use, where there should be a hit.

[Jim Stichnoth, 2014-10-06 23:54:39]

https://codereview.chromium.org/620373004/diff/40001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/620373004/diff/40001/src/IceGlobalContext.cpp...
src/IceGlobalContext.cpp:396: return NumTimerStackIds++;
On 2014/10/06 23:01:52, jvoung wrote:
> Could this just use Timers.size() (or return void)?

Done (and removed the NumTimerStackIds field entirely).

In the imagined future use, the caller would actually save the return value and
use it for the pushTimer and popTimer calls.

https://codereview.chromium.org/620373004/diff/40001/src/IceTimerTree.cpp
File src/IceTimerTree.cpp (right):

https://codereview.chromium.org/620373004/diff/40001/src/IceTimerTree.cpp#new...
src/IceTimerTree.cpp:41: for (auto I = IDs.rbegin(), E = IDs.rend(); I != E;
++I, --Result) {
On 2014/10/06 23:01:52, jvoung wrote:
> I wonder if this should be a map, or unorder_map, now that the main use now is
> for functions (there can be a lot for a linear search)?
> 
> On the other hand, there shouldn't be a hit for functions the way that
> getTimerID() is used, except if it has internal linkage?

Good point, thanks!  Done, by having two containers for quick bi-directional
mapping.  (The original IDs vector keeps dumping simple.)

[jvoung (off chromium), 2014-10-07 15:17:18]

lgtm

[Jim Stichnoth, 2014-10-07 17:02:46]

Committed patchset #4 (id:60001) manually as
8363a0668ce21ea0c2a61f78083b0536dbd89860 (presubmit successful).