Add async trace events to trace progress of IPC messages

Add async trace events to trace progress of IPC messages

BUG=79942
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=154840

[jbates, 2012-08-31 02:06:40]

ptal

This change will allow the about:tracing tool to eventually draw lines from the
original IPC Send call, through each thread to the final receiver thread.

[jbates, 2012-08-31 03:09:33]

Actually, I need to make one change before this works -- need to somehow combine
the PID with the count so that the Message ref-nums are less likely to collide
between processes.

[jbates, 2012-08-31 21:27:11]

jam: ptal

[jam, 2012-08-31 23:07:55]

https://codereview.chromium.org/10919023/diff/6001/ipc/ipc_message.cc
File ipc/ipc_message.cc (right):

https://codereview.chromium.org/10919023/diff/6001/ipc/ipc_message.cc#newcode19
ipc/ipc_message.cc:19: inline uint32 GetRefNumUpper24() {
nit: add a comment

https://codereview.chromium.org/10919023/diff/6001/ipc/ipc_message.cc#newcode41
ipc/ipc_message.cc:41: header()->flags = GetRefNumUpper24();
Are you sure this works with the reference builds? Current performance_ui_tests
binaries talk to old chrome binaries through IPC. So the question is whether the
old version is checking the flags to make sure it's within known values?

https://codereview.chromium.org/10919023/diff/6001/ipc/ipc_message.h
File ipc/ipc_message.h (right):

https://codereview.chromium.org/10919023/diff/6001/ipc/ipc_message.h#newcode229
ipc/ipc_message.h:229: TRACE_EVENT_ASYNC_BEGIN_STEP0("ipc", "IPC",
header()->flags, NULL);
i'm a little worried that this might cause performance regressions? i.e. are
there any locks that this macro cause to be locked? how expensive is this
function? there are tons of ipcs.

[jbates, 2012-08-31 23:27:49]

https://chromiumcodereview.appspot.com/10919023/diff/6001/ipc/ipc_message.cc
File ipc/ipc_message.cc (right):

https://chromiumcodereview.appspot.com/10919023/diff/6001/ipc/ipc_message.cc#...
ipc/ipc_message.cc:19: inline uint32 GetRefNumUpper24() {
On 2012/08/31 23:07:55, John Abd-El-Malek wrote:
> nit: add a comment

Done.

https://chromiumcodereview.appspot.com/10919023/diff/6001/ipc/ipc_message.cc#...
ipc/ipc_message.cc:41: header()->flags = GetRefNumUpper24();
On 2012/08/31 23:07:55, John Abd-El-Malek wrote:
> Are you sure this works with the reference builds? Current
performance_ui_tests
> binaries talk to old chrome binaries through IPC. So the question is whether
the
> old version is checking the flags to make sure it's within known values?

The code that accesses flags in this file just checks for the existing flags in
the first 8 bits. The ipc_tests all pass. Should I look anywhere else for
possible DCHECKs on flags values?

https://chromiumcodereview.appspot.com/10919023/diff/6001/ipc/ipc_message.h
File ipc/ipc_message.h (right):

https://chromiumcodereview.appspot.com/10919023/diff/6001/ipc/ipc_message.h#n...
ipc/ipc_message.h:229: TRACE_EVENT_ASYNC_BEGIN_STEP0("ipc", "IPC",
header()->flags, NULL);
On 2012/08/31 23:07:55, John Abd-El-Malek wrote:
> i'm a little worried that this might cause performance regressions? i.e. are
> there any locks that this macro cause to be locked? how expensive is this
> function? there are tons of ipcs.

When tracing is disabled, it boils down to two if statements on global pointer
values. No locks unless tracing is enable. We already trace every task run in
MessageLoop, for example.

[jam, 2012-09-02 00:35:18]

lgtm

https://codereview.chromium.org/10919023/diff/6001/ipc/ipc_message.cc
File ipc/ipc_message.cc (right):

https://codereview.chromium.org/10919023/diff/6001/ipc/ipc_message.cc#newcode41
ipc/ipc_message.cc:41: header()->flags = GetRefNumUpper24();
On 2012/08/31 23:27:49, jbates wrote:
> On 2012/08/31 23:07:55, John Abd-El-Malek wrote:
> > Are you sure this works with the reference builds? Current
> performance_ui_tests
> > binaries talk to old chrome binaries through IPC. So the question is whether
> the
> > old version is checking the flags to make sure it's within known values?
> 
> The code that accesses flags in this file just checks for the existing flags
in
> the first 8 bits. The ipc_tests all pass. Should I look anywhere else for
> possible DCHECKs on flags values?

you can try running tests which use the reference builds locally, but if by
inspection it looks fine to you, you can just commit and see if the perf bots
blow up

[jbates, 2012-09-04 17:52:41]

bradchen: chrome/nacl/OWNERS

[Mark Seaborn, 2012-09-04 18:39:31]

https://chromiumcodereview.appspot.com/10919023/diff/7002/chrome/nacl/nacl_ip...
File chrome/nacl/nacl_ipc_adapter_unittest.cc (right):

https://chromiumcodereview.appspot.com/10919023/diff/7002/chrome/nacl/nacl_ip...
chrome/nacl/nacl_ipc_adapter_unittest.cc:96: IPC::Message::PRIORITY_MASK |
If someone adds a flag (maybe after removing a flag or changing the
apportionment between the flags and reference number), they're likely not to
know to add it here, so the test would become weaker.

Maybe add an ALL_FLAGS_MASK to ipc_message.h or add a getter function that
extracts the flags?

[jbates, 2012-09-04 20:02:17]

https://chromiumcodereview.appspot.com/10919023/diff/7002/chrome/nacl/nacl_ip...
File chrome/nacl/nacl_ipc_adapter_unittest.cc (right):

https://chromiumcodereview.appspot.com/10919023/diff/7002/chrome/nacl/nacl_ip...
chrome/nacl/nacl_ipc_adapter_unittest.cc:96: IPC::Message::PRIORITY_MASK |
On 2012/09/04 18:39:31, Mark Seaborn wrote:
> If someone adds a flag (maybe after removing a flag or changing the
> apportionment between the flags and reference number), they're likely not to
> know to add it here, so the test would become weaker.
> 
> Maybe add an ALL_FLAGS_MASK to ipc_message.h or add a getter function that
> extracts the flags?

It looks like this test just needs to verify that flags goes through unchanged,
right? I changed the test to get the flags member after constructing the
Message, and then check it afterwards. PTAL.

[Mark Seaborn, 2012-09-04 20:14:05]

On 2012/09/04 20:02:17, jbates wrote:
> It looks like this test just needs to verify that flags goes through
> unchanged, right? I changed the test to get the flags member after
> constructing the Message, and then check it afterwards. PTAL.

Thanks.  LGTM for nacl_ipc_adapter_unittest.cc.