Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1833573002:
Move TRACE_EVENT in IPC message dispatch to add message name (Closed)
|
Created:
4 years, 9 months ago by ssid Modified:
4 years, 8 months ago Reviewers:
Zhen Wang, Primiano Tucci (use gerrit), DmitrySkiba, oystein (OOO til 10th of July), Tom Sepez, epenner CC:
chromium-reviews, darin-cc_chromium.org, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@only_thread_name Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove TRACE_EVENT in IPC message dispatch to add message name
The IPC message names are not displayed unless logging is enabled
(debug builds) in ChannelProxy::Context::OnDispatchMessage. But the
name is actually available in release build binary. Specific message
names are required for memory-infra heap profiler to understand the
memory usage in Chrome, since the task name just shows DispatchMessage
for all ipc messages. More context: https://goo.gl/hcqY5p.
This CL adds the existing names in the chrome binary to tracing when ipc
category is enabled.
BUG=594803
Committed: https://crrev.com/6c1dc894125c68f12d3f14065d057d29c0229860
Cr-Commit-Position: refs/heads/master@{#383546}
Patch Set 1 #Patch Set 2 : remove previous trace event. #Patch Set 3 : Remove patch deppendency. #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 47 (19 generated)
ssid@chromium.org changed reviewers: + dskiba@chromium.org
Do you think it's a fair change given that https://chromium.googlesource.com/chromium/src/+/master/ipc/ipc_channel_proxy... exists only on debug builds.
Description was changed from ========== Add IPC message name to tracing The IPC message names are not displayed unless logging is enabled. This CL adds the already existing name in the chrome binary to tracing when ipc category is enabled. BUG=594803 ========== to ========== Add IPC message name to tracing The IPC message names are not displayed unless logging is enabled (debug builds) in ChannelProxy::Context::OnDispatchMessage. But the name is actually available in release build binary. Specific message names are required for memory-infra heap profiler to understand the memory usage in Chrome, since the task name just shows DispatchMessage for all ipc messages. More context: https://goo.gl/hcqY5p. This CL adds the existing names in the chrome binary to tracing when ipc category is enabled. BUG=594803 ==========
I'm wondering how having trace macro there impacts performance. Because even if "ipc" tracing is not enabled, that macro still does something, right? Can we run this on perf bots somehow?
On 2016/03/24 20:02:49, Dmitry Skiba wrote: > I'm wondering how having trace macro there impacts performance. Because even if > "ipc" tracing is not enabled, that macro still does something, right? Can we run > this on perf bots somehow? We already have one trace macro on each ipc message at the link i posted. I am sure it doesn't cause any perf regression since these macros are present everywhere in codebase.
On 2016/03/24 21:53:16, ssid wrote: > On 2016/03/24 20:02:49, Dmitry Skiba wrote: > > I'm wondering how having trace macro there impacts performance. Because even > if > > "ipc" tracing is not enabled, that macro still does something, right? Can we > run > > this on perf bots somehow? > > We already have one trace macro on each ipc message at the link i posted. I am > sure it doesn't cause any perf regression since these macros are present > everywhere in codebase. Yeah, but that one is guarded by IPC_MESSAGE_LOG_ENABLED, which is not defined in release builds. Chrome actually sends/receives a lot of IPC messages, so I think there is a real chance for a regression.
On 2016/03/24 22:02:30, Dmitry Skiba wrote: > On 2016/03/24 21:53:16, ssid wrote: > > On 2016/03/24 20:02:49, Dmitry Skiba wrote: > > > I'm wondering how having trace macro there impacts performance. Because even > > if > > > "ipc" tracing is not enabled, that macro still does something, right? Can we > > run > > > this on perf bots somehow? > > > > We already have one trace macro on each ipc message at the link i posted. I am > > sure it doesn't cause any perf regression since these macros are present > > everywhere in codebase. > > Yeah, but that one is guarded by IPC_MESSAGE_LOG_ENABLED, which is not defined > in release builds. Chrome actually sends/receives a lot of IPC messages, so I > think there is a real chance for a regression. No, there are two trace events added the guard is to choose which one to use. There is always one trace event added. Check the #else condition. Anyway if you feel there might be a regression I will have an eye few perf graphs and make sure everything is fine after it lands.
ssid@chromium.org changed reviewers: + zhenw@chromium.org
+zhenw I think you care about the number of trace events flying around in code. I am planning to add one more event per each ipc. what do you think? Can you suggest a better person to ask if not you? If can't add this event, then I should find some other way to insert and remove from the profiler pseudo stack without adding event.
Description was changed from ========== Add IPC message name to tracing The IPC message names are not displayed unless logging is enabled (debug builds) in ChannelProxy::Context::OnDispatchMessage. But the name is actually available in release build binary. Specific message names are required for memory-infra heap profiler to understand the memory usage in Chrome, since the task name just shows DispatchMessage for all ipc messages. More context: https://goo.gl/hcqY5p. This CL adds the existing names in the chrome binary to tracing when ipc category is enabled. BUG=594803 ========== to ========== Add TRACE_EVENT around IPC message handler with name The IPC message names are not displayed unless logging is enabled (debug builds) in ChannelProxy::Context::OnDispatchMessage. But the name is actually available in release build binary. Specific message names are required for memory-infra heap profiler to understand the memory usage in Chrome, since the task name just shows DispatchMessage for all ipc messages. More context: https://goo.gl/hcqY5p. This CL adds the existing names in the chrome binary to tracing when ipc category is enabled. BUG=594803 ==========
zhenw@chromium.org changed reviewers: + oysteine@chromium.org, primiano@chromium.org
+primiano and oysteine I imagine tracing each IPC will add many events?
On 2016/03/25 20:36:19, Zhen Wang wrote: > +primiano and oysteine > > I imagine tracing each IPC will add many events? Yes, it does add many events. Also there is a trace event added here per message: https://chromium.googlesource.com/chromium/src/+/master/ipc/ipc_channel_proxy...
On 2016/03/25 20:37:59, ssid wrote: > On 2016/03/25 20:36:19, Zhen Wang wrote: > > +primiano and oysteine > > > > I imagine tracing each IPC will add many events? > > Yes, it does add many events. > Also there is a trace event added here per message: > https://chromium.googlesource.com/chromium/src/+/master/ipc/ipc_channel_proxy... I will delegate this to primiano and oysteine.
On 2016/03/25 20:42:30, Zhen Wang wrote: > On 2016/03/25 20:37:59, ssid wrote: > > On 2016/03/25 20:36:19, Zhen Wang wrote: > > > +primiano and oysteine > > > > > > I imagine tracing each IPC will add many events? > > > > Yes, it does add many events. > > Also there is a trace event added here per message: > > > https://chromium.googlesource.com/chromium/src/+/master/ipc/ipc_channel_proxy... > > I will delegate this to primiano and oysteine. 'ipc' is an enabled-by-default category, so I'm wary of adding this amount of new trace events to it. Can you change it to disabled-by-default-ipc instead?
On 2016/03/25 20:48:49, Oystein wrote: > On 2016/03/25 20:42:30, Zhen Wang wrote: > > On 2016/03/25 20:37:59, ssid wrote: > > > On 2016/03/25 20:36:19, Zhen Wang wrote: > > > > +primiano and oysteine > > > > > > > > I imagine tracing each IPC will add many events? > > > > > > Yes, it does add many events. > > > Also there is a trace event added here per message: > > > > > > https://chromium.googlesource.com/chromium/src/+/master/ipc/ipc_channel_proxy... > > > > I will delegate this to primiano and oysteine. > > 'ipc' is an enabled-by-default category, so I'm wary of adding this amount of > new trace events to it. Can you change it to disabled-by-default-ipc instead? Do you think it's fine if I remove the trace events at this place: https://chromium.googlesource.com/chromium/src/+/master/ipc/ipc_channel_proxy... since this is more useful.
On 2016/03/25 20:50:27, ssid wrote: > On 2016/03/25 20:48:49, Oystein wrote: > > On 2016/03/25 20:42:30, Zhen Wang wrote: > > > On 2016/03/25 20:37:59, ssid wrote: > > > > On 2016/03/25 20:36:19, Zhen Wang wrote: > > > > > +primiano and oysteine > > > > > > > > > > I imagine tracing each IPC will add many events? > > > > > > > > Yes, it does add many events. > > > > Also there is a trace event added here per message: > > > > > > > > > > https://chromium.googlesource.com/chromium/src/+/master/ipc/ipc_channel_proxy... > > > > > > I will delegate this to primiano and oysteine. > > > > 'ipc' is an enabled-by-default category, so I'm wary of adding this amount of > > new trace events to it. Can you change it to disabled-by-default-ipc instead? > > Do you think it's fine if I remove the trace events at this place: > https://chromium.googlesource.com/chromium/src/+/master/ipc/ipc_channel_proxy... > since this is more useful. Yep that seems absolutely fine; I can't find anything that depends on the name (and there's an entry in trace_event_args_whitelist.cc you can remove, since the IPC name is now the event name).
Patchset #2 (id:20001) has been deleted
ssid@chromium.org changed reviewers: + epenner@chromium.org, rbyers@chromium.org, tsepez@chromium.org
+tsepez for changes in ipc/ This moves the trace event in dispatch to show better names. +rbyers, epenner Who had previously changed these trace events.
Are we sure there is no performance hit for doing this on every dispatch?
chrome/common/trace_event_args_whitelist.cc lgtm
On 2016/03/25 21:33:15, Tom Sepez wrote: > Are we sure there is no performance hit for doing this on every dispatch? There were events already around each dispatch, I am just moving it to better place. Sorry for not being clear.
On 2016/03/25 21:33:15, Tom Sepez wrote: > Are we sure there is no performance hit for doing this on every dispatch? There were events already around each dispatch, I am just moving it to better place. Sorry for not being clear.
On 2016/03/25 21:33:15, Tom Sepez wrote: > Are we sure there is no performance hit for doing this on every dispatch? Also, maybe we should try to get these string constants out of a release binary? Might save on the order of 100kb?
On 2016/03/25 21:36:52, Tom Sepez wrote: > On 2016/03/25 21:33:15, Tom Sepez wrote: > > Are we sure there is no performance hit for doing this on every dispatch? > Also, maybe we should try to get these string constants out of a release binary? > Might save on the order of 100kb? lgtm
Description was changed from ========== Add TRACE_EVENT around IPC message handler with name The IPC message names are not displayed unless logging is enabled (debug builds) in ChannelProxy::Context::OnDispatchMessage. But the name is actually available in release build binary. Specific message names are required for memory-infra heap profiler to understand the memory usage in Chrome, since the task name just shows DispatchMessage for all ipc messages. More context: https://goo.gl/hcqY5p. This CL adds the existing names in the chrome binary to tracing when ipc category is enabled. BUG=594803 ========== to ========== Move TRACE_EVENT in IPC message dispatch to add message name The IPC message names are not displayed unless logging is enabled (debug builds) in ChannelProxy::Context::OnDispatchMessage. But the name is actually available in release build binary. Specific message names are required for memory-infra heap profiler to understand the memory usage in Chrome, since the task name just shows DispatchMessage for all ipc messages. More context: https://goo.gl/hcqY5p. This CL adds the existing names in the chrome binary to tracing when ipc category is enabled. BUG=594803 ==========
I don't really have much context here, so I'll leave the review to IPC owners. But when I changed this a couple years back we didn't actually have the IPC name strings in the release binary (perhaps for binary size reasons?). I assume, by adding kName references outside of DEBUG-only macros, you're not changing that to include strings that wouldn't otherwise be in the binary, are you?
rbyers@chromium.org changed reviewers: - rbyers@chromium.org
On 2016/03/25 21:40:15, Rick Byers wrote: > I don't really have much context here, so I'll leave the review to IPC owners. > But when I changed this a couple years back we didn't actually have the IPC name > strings in the release binary (perhaps for binary size reasons?). I assume, by > adding kName references outside of DEBUG-only macros, you're not changing that > to include strings that wouldn't otherwise be in the binary, are you? Oh sorry, I see you've covered this in the CL description. If the names are already in the binary, then great!
On 2016/03/25 21:42:40, Rick Byers wrote: > On 2016/03/25 21:40:15, Rick Byers wrote: > > I don't really have much context here, so I'll leave the review to IPC owners. > > > But when I changed this a couple years back we didn't actually have the IPC > name > > strings in the release binary (perhaps for binary size reasons?). I assume, > by > > adding kName references outside of DEBUG-only macros, you're not changing that > > to include strings that wouldn't otherwise be in the binary, are you? > > Oh sorry, I see you've covered this in the CL description. If the names are > already in the binary, then great! Yes I am not adding these strings newly. Just using the existing strings. I will try to find out who added these and how much was size increase and see if we can reduce them. Till them I will prefer if tracing uses these. I wanted to add you as cc/ to let you know I am changing what you added. but mistakenly added as reviewer sorry. Thanks for all the comments.
The CQ bit was checked by ssid@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1833573002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1833573002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by ssid@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from oysteine@chromium.org, tsepez@chromium.org Link to the patchset: https://codereview.chromium.org/1833573002/#ps60001 (title: "Fix builds.")
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 1814083002 Patch 100001). Please resolve the dependency and try again.
The CQ bit was checked by ssid@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from oysteine@chromium.org, tsepez@chromium.org Link to the patchset: https://codereview.chromium.org/1833573002/#ps80001 (title: "Remove patch deppendency.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1833573002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1833573002/80001
Message was sent while issue was closed.
Description was changed from ========== Move TRACE_EVENT in IPC message dispatch to add message name The IPC message names are not displayed unless logging is enabled (debug builds) in ChannelProxy::Context::OnDispatchMessage. But the name is actually available in release build binary. Specific message names are required for memory-infra heap profiler to understand the memory usage in Chrome, since the task name just shows DispatchMessage for all ipc messages. More context: https://goo.gl/hcqY5p. This CL adds the existing names in the chrome binary to tracing when ipc category is enabled. BUG=594803 ========== to ========== Move TRACE_EVENT in IPC message dispatch to add message name The IPC message names are not displayed unless logging is enabled (debug builds) in ChannelProxy::Context::OnDispatchMessage. But the name is actually available in release build binary. Specific message names are required for memory-infra heap profiler to understand the memory usage in Chrome, since the task name just shows DispatchMessage for all ipc messages. More context: https://goo.gl/hcqY5p. This CL adds the existing names in the chrome binary to tracing when ipc category is enabled. BUG=594803 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Move TRACE_EVENT in IPC message dispatch to add message name The IPC message names are not displayed unless logging is enabled (debug builds) in ChannelProxy::Context::OnDispatchMessage. But the name is actually available in release build binary. Specific message names are required for memory-infra heap profiler to understand the memory usage in Chrome, since the task name just shows DispatchMessage for all ipc messages. More context: https://goo.gl/hcqY5p. This CL adds the existing names in the chrome binary to tracing when ipc category is enabled. BUG=594803 ========== to ========== Move TRACE_EVENT in IPC message dispatch to add message name The IPC message names are not displayed unless logging is enabled (debug builds) in ChannelProxy::Context::OnDispatchMessage. But the name is actually available in release build binary. Specific message names are required for memory-infra heap profiler to understand the memory usage in Chrome, since the task name just shows DispatchMessage for all ipc messages. More context: https://goo.gl/hcqY5p. This CL adds the existing names in the chrome binary to tracing when ipc category is enabled. BUG=594803 Committed: https://crrev.com/6c1dc894125c68f12d3f14065d057d29c0229860 Cr-Commit-Position: refs/heads/master@{#383546} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/6c1dc894125c68f12d3f14065d057d29c0229860 Cr-Commit-Position: refs/heads/master@{#383546}
Message was sent while issue was closed.
Patchset #3 (id:60001) has been deleted |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
