Subzero: Fix timers for multithreaded translation.

Subzero: Fix timers for multithreaded translation.

Now that multithreaded parsing and translation is in place, timer operations have to be made thread-local.  After the non-main threads end, their thread-local timer data needs to be merged into the global timer data, which resides in the GlobalContext object.  The merge is a bit tricky because the internal timer stack structure is built up dynamically as items are pushed and popped.  Two threads may have radically different timing data:

1. The parser thread profile is completely different from a translator thread.

2. For -timing-funcs, two translator threads hold data for entirely different sets of functions.

A bit more tweaking will need to be done to make the timing output fully usable in a multithreaded run.  Because of multiple threads, times may add up to >100%.  Also, time spent blocked is being "unfairly" attributed to the caller of the blocking operation - we should either count the user time instead of wall-clock time, or add a special timer marker for blocking locking operations.

BUG= none
R=jvoung@chromium.org

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

[Jim Stichnoth, 2015-01-30 05:14:13]

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

[jvoung (off chromium), 2015-01-30 18:42:56]

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

https://codereview.chromium.org/878383004/diff/20001/src/IceGlobalContext.cpp...
src/IceGlobalContext.cpp:564: Ctx->pushTimer(ID);
Should this pushTimer be given the StackID too?

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

https://codereview.chromium.org/878383004/diff/20001/src/IceGlobalContext.h#n...
src/IceGlobalContext.h:404: : ID(ID), Ctx(Ctx), StackID(StackID), Active(false)
{
I wonder if this ctor should be defined out of line now. Then it could be next
to the Cfg version.

https://codereview.chromium.org/878383004/diff/20001/src/IceTimerTree.cpp
File src/IceTimerTree.cpp (right):

https://codereview.chromium.org/878383004/diff/20001/src/IceTimerTree.cpp#new...
src/IceTimerTree.cpp:73: TTindex Index = 0;
So the TimerIDs might need mapping, but the TTindexes should be 1:1?

https://codereview.chromium.org/878383004/diff/20001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/878383004/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1830: delete Func;
Maybe Func should be a smart pointer, and the call to translateFcn should more
explicitly release ownership.

[Jim Stichnoth, 2015-01-30 20:22:25]

Thanks!

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

https://codereview.chromium.org/878383004/diff/20001/src/IceGlobalContext.cpp...
src/IceGlobalContext.cpp:564: Ctx->pushTimer(ID);
On 2015/01/30 18:42:56, jvoung wrote:
> Should this pushTimer be given the StackID too?

Oops yes, thanks!

I removed the default parameter from pushTimer() and popTimer() to help prevent
this mistake in the future.

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

https://codereview.chromium.org/878383004/diff/20001/src/IceGlobalContext.h#n...
src/IceGlobalContext.h:404: : ID(ID), Ctx(Ctx), StackID(StackID), Active(false)
{
On 2015/01/30 18:42:56, jvoung wrote:
> I wonder if this ctor should be defined out of line now. Then it could be next
> to the Cfg version.

I originally wanted both ctors (and the dtor) to be inline, so that with
ALLOW_DUMP #defined to 1 in the minimal build, the code would collapse to
nothing.  That unfortunately requires including IceCfg.h which is kind of icky,
so the Cfg version is defined out of line.

I think I rearranged things to get the best of both worlds.  The ALLOW_DUMP
portions of both ctors are inline, and the rest is done out of line.

https://codereview.chromium.org/878383004/diff/20001/src/IceTimerTree.cpp
File src/IceTimerTree.cpp (right):

https://codereview.chromium.org/878383004/diff/20001/src/IceTimerTree.cpp#new...
src/IceTimerTree.cpp:73: TTindex Index = 0;
On 2015/01/30 18:42:56, jvoung wrote:
> So the TimerIDs might need mapping, but the TTindexes should be 1:1? 

No, they need mapping too.

The "Index" variable is with respect to the Src timer stack.  (I renamed Index
to SrcIndex to make that clear.)  Then getPath() explodes the Src index into its
path, and applies Mapping to get a path in terms of "my" timer stack.  (I
renamed Path to MyPath to make that clear.)  Finally, findPath() returns the
node index for "my" timer stack.

(I also renamed Node to SrcNode for clarity.)

https://codereview.chromium.org/878383004/diff/20001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/878383004/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1830: delete Func;
On 2015/01/30 18:42:56, jvoung wrote:
> Maybe Func should be a smart pointer, and the call to translateFcn should more
> explicitly release ownership.

Yeah, there's a TODO for that in IceCfg.h.

Also,
https://codereview.chromium.org/870653002/diff/20001/src/IceGlobalContext.cpp...

:)

(I'd like to plumb that in a separate CL.)

[jvoung (off chromium), 2015-01-30 21:04:44]

lgtm

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

https://codereview.chromium.org/878383004/diff/20001/src/IceGlobalContext.h#n...
src/IceGlobalContext.h:404: : ID(ID), Ctx(Ctx), StackID(StackID), Active(false)
{
On 2015/01/30 20:22:25, stichnot wrote:
> On 2015/01/30 18:42:56, jvoung wrote:
> > I wonder if this ctor should be defined out of line now. Then it could be
next
> > to the Cfg version.
> 
> I originally wanted both ctors (and the dtor) to be inline, so that with
> ALLOW_DUMP #defined to 1 in the minimal build, the code would collapse to
> nothing.  That unfortunately requires including IceCfg.h which is kind of
icky,
> so the Cfg version is defined out of line.
> 
> I think I rearranged things to get the best of both worlds.  The ALLOW_DUMP
> portions of both ctors are inline, and the rest is done out of line.

I see okay.

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

https://codereview.chromium.org/878383004/diff/40001/src/IceGlobalContext.cpp...
src/IceGlobalContext.cpp:565: Active = Ctx->getFlags().TimeEachFunction;
add a break here too just in case?

[Jim Stichnoth, 2015-01-30 21:10:06]

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

https://codereview.chromium.org/878383004/diff/40001/src/IceGlobalContext.cpp...
src/IceGlobalContext.cpp:565: Active = Ctx->getFlags().TimeEachFunction;
On 2015/01/30 21:04:44, jvoung wrote:
> add a break here too just in case?

yikes, thanks!

[Jim Stichnoth, 2015-01-30 21:10:43]

Committed patchset #4 (id:60001) manually as
380d7b96c63acc79b84a08bb24ae1ffcd132a0be (presubmit successful).