Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2374193002:
Make base::PendingTask support ConvertableToTraceFormat (Closed)
DescriptionMake base::PendingTask support ConvertableToTraceFormat
in order for it to only use up a single trace event parameter slot.
BUG=649084, 652692
TEST=Take a trace and confirm "src_info" is in args of toplevel tasks.
Patch Set 1 #Patch Set 2 : JsonWriter::Write requires a temp var #
Total comments: 7
Dependent Patchsets: Messages
Total messages: 32 (20 generated)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:20001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Make base::PendingTask support ConvertableToTraceFormat in order for it to only use up a single trace event parameter slot. BUG=649084 ========== to ========== Make base::PendingTask support ConvertableToTraceFormat in order for it to only use up a single trace event parameter slot. BUG=649084 TEST=Take a trace and confirm "src_info" is in args of toplevel tasks. ==========
gab@chromium.org changed reviewers: + oysteine@chromium.org
Oysteine PTAL, seems to work, one bad side-effect I noticed is that searching for a src_func (e.g. "ScheduleBeginFrameDeadline") in the chrome://tracing UI no longer finds it... I'm not sure why since search before this CL was able to find partial method names, does it not search through JSON objects dumped by convertable objs?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/09/29 14:43:33, gab wrote: > Oysteine PTAL, seems to work, one bad side-effect I noticed is that searching > for a src_func (e.g. "ScheduleBeginFrameDeadline") in the chrome://tracing UI no > longer finds it... I'm not sure why since search before this CL was able to find > partial method names, does it not search through JSON objects dumped by > convertable objs? This is going to cause heap traffic on each message loop task. What is wrong with the current TRACE_EVENT2? The cl description says "in order for it to only use up a single trace event parameter slot.". Why is that a problem? Also there is no clear explanation why this is only for dcheck_always_on builds. Maybe there is a good reason for this, but definitely is not well communicated.
On 2016/09/29 17:13:03, Primiano Tucci wrote: > On 2016/09/29 14:43:33, gab wrote: > > Oysteine PTAL, seems to work, one bad side-effect I noticed is that searching > > for a src_func (e.g. "ScheduleBeginFrameDeadline") in the chrome://tracing UI > no > > longer finds it... I'm not sure why since search before this CL was able to > find > > partial method names, does it not search through JSON objects dumped by > > convertable objs? > > This is going to cause heap traffic on each message loop task. What is wrong > with the current TRACE_EVENT2? > The cl description says "in order for it to only use up a single trace event > parameter slot.". Why is that a problem? > Also there is no clear explanation why this is only for dcheck_always_on builds. > Maybe there is a good reason for this, but definitely is not well communicated. The linked bug has some discussion that should probably be added to the CL description, but I think as it is the main thing missing based on the discussion there is that the convertible shouldn't be added unless disabled-by-default.toplevel is set.
On 2016/09/29 17:37:10, oystein wrote: > On 2016/09/29 17:13:03, Primiano Tucci wrote: > > On 2016/09/29 14:43:33, gab wrote: > > > Oysteine PTAL, seems to work, one bad side-effect I noticed is that > searching > > > for a src_func (e.g. "ScheduleBeginFrameDeadline") in the chrome://tracing > UI > > no > > > longer finds it... I'm not sure why since search before this CL was able to > > find > > > partial method names, does it not search through JSON objects dumped by > > > convertable objs? > > > > This is going to cause heap traffic on each message loop task. What is wrong > > with the current TRACE_EVENT2? TRACE_EVENT2 (used by TRACE_TASK_EXECUTION) uses both parameters available in AddTraceEvent(). This is problematic as post task sites like the TaskScheduler can't add more info to tasks. Making AddTraceEvent() take more parameters isn't really an option. As such merging src_file/src_func into a single parameter (via a ConvertableToTraceFormat object) was discussed on the bug as the best alternative. I agree that this causing heap churn isn't great. I wish that types which implement ConvertableToTraceFormat could be passed by reference instead of by std::unique_ptr but this isn't the case (also discussed on the bug -- reason is thread-safety for ring-buffer tracing which only optionally dumps later, in this case though we copy the required strings into a heap object, so tracing could just as well take the ConvertableToTraceFormat as a const&, extract the string and copy it around itself...). > > The cl description says "in order for it to only use up a single trace event > > parameter slot.". Why is that a problem? > > Also there is no clear explanation why this is only for dcheck_always_on > builds. > > Maybe there is a good reason for this, but definitely is not well > communicated. > > The linked bug has some discussion that should probably be added to the CL > description, but I think as it is the main thing missing based on the discussion > there is that the convertible shouldn't be added unless > disabled-by-default.toplevel is set. The way I see it, making src_file/src_func not be reported by default is orthogonal to how it is reported. I'm up for making the change to allow more things to be reported, but I'm not up for changing the default as suggested (as it is completely orthogonal to what I'm trying to achieve and will likely affect other systems so I'd prefer tracing owners to do it as they see fit).
On 2016/09/29 19:07:50, gab wrote: > On 2016/09/29 17:37:10, oystein wrote: > > On 2016/09/29 17:13:03, Primiano Tucci wrote: > > > On 2016/09/29 14:43:33, gab wrote: > > > > Oysteine PTAL, seems to work, one bad side-effect I noticed is that > > searching > > > > for a src_func (e.g. "ScheduleBeginFrameDeadline") in the chrome://tracing > > UI > > > no > > > > longer finds it... I'm not sure why since search before this CL was able > to > > > find > > > > partial method names, does it not search through JSON objects dumped by > > > > convertable objs? > > > > > > This is going to cause heap traffic on each message loop task. What is wrong > > > with the current TRACE_EVENT2? > > TRACE_EVENT2 (used by TRACE_TASK_EXECUTION) uses both parameters available in > AddTraceEvent(). > This is problematic as post task sites like the TaskScheduler can't add more > info to tasks. > > Making AddTraceEvent() take more parameters isn't really an option. As such > merging src_file/src_func into a single parameter (via a > ConvertableToTraceFormat object) was discussed on the bug as the best > alternative. > > I agree that this causing heap churn isn't great. I wish that types which > implement ConvertableToTraceFormat could be passed by reference instead of by > std::unique_ptr but this isn't the case (also discussed on the bug -- reason is > thread-safety for ring-buffer tracing which only optionally dumps later, in this > case though we copy the required strings into a heap object, so tracing could > just as well take the ConvertableToTraceFormat as a const&, extract the string > and copy it around itself...). > > > > The cl description says "in order for it to only use up a single trace event > > > parameter slot.". Why is that a problem? > > > Also there is no clear explanation why this is only for dcheck_always_on > > builds. > > > Maybe there is a good reason for this, but definitely is not well > > communicated. > > > > The linked bug has some discussion that should probably be added to the CL > > description, but I think as it is the main thing missing based on the > discussion > > there is that the convertible shouldn't be added unless > > disabled-by-default.toplevel is set. > > The way I see it, making src_file/src_func not be reported by default is > orthogonal to how it is reported. > > I'm up for making the change to allow more things to be reported, but I'm not up > for changing the default as suggested (as it is completely orthogonal to what > I'm trying to achieve and will likely affect other systems so I'd prefer tracing > owners to do it as they see fit). That's completely understandable, my point here is the main concern I raised in the bug: "toplevel" is already causing problems on the perf waterfall due to the size and frequency of those events, and we can't add more strings to that category by default (i.e. without going via a disabled-by-default.toplevel check).
On 2016/09/29 19:23:50, oystein wrote: > On 2016/09/29 19:07:50, gab wrote: > > On 2016/09/29 17:37:10, oystein wrote: > > > On 2016/09/29 17:13:03, Primiano Tucci wrote: > > > > On 2016/09/29 14:43:33, gab wrote: > > > > > Oysteine PTAL, seems to work, one bad side-effect I noticed is that > > > searching > > > > > for a src_func (e.g. "ScheduleBeginFrameDeadline") in the > chrome://tracing > > > UI > > > > no > > > > > longer finds it... I'm not sure why since search before this CL was able > > to > > > > find > > > > > partial method names, does it not search through JSON objects dumped by > > > > > convertable objs? > > > > > > > > This is going to cause heap traffic on each message loop task. What is > wrong > > > > with the current TRACE_EVENT2? > > > > TRACE_EVENT2 (used by TRACE_TASK_EXECUTION) uses both parameters available in > > AddTraceEvent(). > > This is problematic as post task sites like the TaskScheduler can't add more > > info to tasks. > > > > Making AddTraceEvent() take more parameters isn't really an option. As such > > merging src_file/src_func into a single parameter (via a > > ConvertableToTraceFormat object) was discussed on the bug as the best > > alternative. > > > > I agree that this causing heap churn isn't great. I wish that types which > > implement ConvertableToTraceFormat could be passed by reference instead of by > > std::unique_ptr but this isn't the case (also discussed on the bug -- reason > is > > thread-safety for ring-buffer tracing which only optionally dumps later, in > this > > case though we copy the required strings into a heap object, so tracing could > > just as well take the ConvertableToTraceFormat as a const&, extract the string > > and copy it around itself...). > > > > > > The cl description says "in order for it to only use up a single trace > event > > > > parameter slot.". Why is that a problem? > > > > Also there is no clear explanation why this is only for dcheck_always_on > > > builds. > > > > Maybe there is a good reason for this, but definitely is not well > > > communicated. > > > > > > The linked bug has some discussion that should probably be added to the CL > > > description, but I think as it is the main thing missing based on the > > discussion > > > there is that the convertible shouldn't be added unless > > > disabled-by-default.toplevel is set. > > > > The way I see it, making src_file/src_func not be reported by default is > > orthogonal to how it is reported. > > > > I'm up for making the change to allow more things to be reported, but I'm not > up > > for changing the default as suggested (as it is completely orthogonal to what > > I'm trying to achieve and will likely affect other systems so I'd prefer > tracing > > owners to do it as they see fit). > > That's completely understandable, my point here is the main concern I raised in > the bug: "toplevel" is already causing problems on the perf waterfall due to the > size and frequency of those events, and we can't add more strings to that > category by default (i.e. without going via a disabled-by-default.toplevel > check). Right, I do intend for the extra args I plan to add after this CL to be behind a disabled-by-default category. But this CL shouldn't grow the current output, it merely moves two args into a single JSON arg.
caseq@chromium.org changed reviewers: + caseq@chromium.org
https://codereview.chromium.org/2374193002/diff/60001/base/pending_task.cc File base/pending_task.cc (right): https://codereview.chromium.org/2374193002/diff/60001/base/pending_task.cc#ne... base/pending_task.cc:22: bool tracing_enabled; considering this is hot code, you really want to keep a static pointer to the category, rather than do category lookup each time. https://codereview.chromium.org/2374193002/diff/60001/base/pending_task.h File base/pending_task.h (right): https://codereview.chromium.org/2374193002/diff/60001/base/pending_task.h#new... base/pending_task.h:25: class TracingInfo : public trace_event::ConvertableToTraceFormat { Why does it have to be nested class of PendingTask? This could as well be somewhere else. https://codereview.chromium.org/2374193002/diff/60001/base/pending_task.h#new... base/pending_task.h:53: std::unique_ptr<trace_event::ConvertableToTraceFormat> GetTracingInfo() const; ditto.
gab@chromium.org changed reviewers: + primiano@chromium.org
https://codereview.chromium.org/2374193002/diff/60001/base/pending_task.cc File base/pending_task.cc (right): https://codereview.chromium.org/2374193002/diff/60001/base/pending_task.cc#ne... base/pending_task.cc:22: bool tracing_enabled; On 2016/09/29 22:46:36, caseq wrote: > considering this is hot code, you really want to keep a static pointer to the > category, rather than do category lookup each time. This is only hot code if both (1) dchecks are enabled and (2) tracing is enabled (as verified by this very dcheck). Which IMO means a developer who's testing tracing (not using it for performance analysis as it doesn't make much sense to do this in a trunk build with DCHECKs). As such I think correctness is superior to performance here and TRACE_EVENT_CATEGORY_GROUP_ENABLED() also isn't much of a performance issue when tracing is enabled as it's called all over the place in that case. https://codereview.chromium.org/2374193002/diff/60001/base/pending_task.h File base/pending_task.h (right): https://codereview.chromium.org/2374193002/diff/60001/base/pending_task.h#new... base/pending_task.h:25: class TracingInfo : public trace_event::ConvertableToTraceFormat { On 2016/09/29 22:46:37, caseq wrote: > Why does it have to be nested class of PendingTask? This could as well be > somewhere else. Good point, I'll move it when we achieve overall consensus on what this CL is doing. https://codereview.chromium.org/2374193002/diff/60001/base/pending_task.h#new... base/pending_task.h:53: std::unique_ptr<trace_event::ConvertableToTraceFormat> GetTracingInfo() const; On 2016/09/29 22:46:37, caseq wrote: > ditto. Acknowledged.
On 2016/10/03 15:18:58, gab wrote: > > As such I think correctness is superior to performance here and > TRACE_EVENT_CATEGORY_GROUP_ENABLED() also isn't much of a performance issue when > tracing is enabled as it's called all over the place in that case. Ah, it actually keeps the static pointer to category group already, so it should be fine, we only do category lookup once. My bad here.
So remaining points of contention here I think:
1) primiano: "This is going to cause heap traffic on each task":
True, the alternatives are:
a) Allow ConvertableToTraceFormat to be passed by value (and make PendingTask
inherit ConvertableToTraceFormat directly).
For thread-safety, the tracing system would have to extract the string
synchronously (which would result in the same number of bytes being copied as
today where two strings are passed by value).
b) Add "std::string PendingTask::GetTracingInfo()" and hand a string directly
to the tracing system that contains both src_file and src_func.
2) oysteine: wanting to make src_file/src_func not reported by default.
This CL, if anything, makes that easier to do in a follow-up CL, but I won't do
it as part of this one (follow-up info to be added in future CLs will themselves
be behind disabled-by-default categories).
skyostil@chromium.org changed reviewers: + skyostil@chromium.org
https://codereview.chromium.org/2374193002/diff/60001/base/trace_event/trace_... File base/trace_event/trace_event.h (right): https://codereview.chromium.org/2374193002/diff/60001/base/trace_event/trace_... base/trace_event/trace_event.h:441: #define INTERNAL_TRACE_TASK_EXECUTION(run_function, task) \ I believe there are a number of tools (e.g., Lighthouse, maybe also Telemetry) that parse this information. Could we try to track them down and coordinate this change with them?
On 2016/10/04 11:25:31, Sami wrote: > https://codereview.chromium.org/2374193002/diff/60001/base/trace_event/trace_... > File base/trace_event/trace_event.h (right): > > https://codereview.chromium.org/2374193002/diff/60001/base/trace_event/trace_... > base/trace_event/trace_event.h:441: #define > INTERNAL_TRACE_TASK_EXECUTION(run_function, task) \ > I believe there are a number of tools (e.g., Lighthouse, maybe also Telemetry) > that parse this information. Could we try to track them down and coordinate this > change with them? Right. Thanks everyone for you input, I'll go ahead and close this CL and work around the current limitations by logging a second trace event for the same task when "task_scheduler" tracing category is enabled. I filed http://crbug.com/652692 to track the improvements discussed here but will let the tracing owners decide where to go from here.
Message was sent while issue was closed.
Description was changed from ========== Make base::PendingTask support ConvertableToTraceFormat in order for it to only use up a single trace event parameter slot. BUG=649084 TEST=Take a trace and confirm "src_info" is in args of toplevel tasks. ========== to ========== Make base::PendingTask support ConvertableToTraceFormat in order for it to only use up a single trace event parameter slot. BUG=649084, 652692 TEST=Take a trace and confirm "src_info" is in args of toplevel tasks. ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
