@dsinclair in your quality of base\trace_event\OWNERS (and also looks like you added convertable formats in 2019248a4):

So I tried to do this in https://codereview.chromium.org/2356393002 but I get:

src\base\task_scheduler\task_tracker.cc(237): error C2660: 'trace_event_internal::AddTraceEvent': function does not take 13 arguments

The issue is that there is no AddTraceEvent method that takes more than 2 extra args. I tried to update this but the current TRACE_VALUE_TYPE_CONVERTABLE template-based system makes this fairly inflexible.

My question is whether you see another way than the current TRACE_VALUE_TYPE_CONVERTABLE approach as that makes it very hard to support an arg count beyond two (combinatorial explosion of template types required to support ARG_TYPE versus ARG_CONVERTABLE_TYPE in trace_event.h)?

Thanks!
Gab

+dsinclair for realz, see above question about TRACE_VALUE_TYPE_CONVERTABLE in trace_event.h, thanks!

Moving to more then two args could be difficult as there, at least were, a bunch of places expecting it to be two.

The usual way to do this would be to use a convertable object, although, maybe that's changing with TracingV2. Adding oysteine@ to see if that's the case.

I'm not sure what you mean with convertable making it hard to have an arg count > 2, with the convertable you wouldn't need that many args as the data would all be in the convertable object, no?

What I meant is that AddTraceEvent has 4 template definitions to support all combinations of {ARG_TYPE, ARG_CONVERTALE_TYPE} if the number of args grows, this becomes 2^n template definitions.

Good point that I could pack everything into a single convertable object, hadn't thought of that but that's a great option :-).

One concern I have is the toplevel tasks trace events are already super spammy, and we're having significant problems with the size of traces on the waterfall and running into JS string size limits pretty much exclusively because of it. I don't know if increasing it further is really a good idea. One solution would be to add a disabled-by-default event which contains all of the extra debug data and could be used for the manual debugging cases.

For the specific question: Yeah, convertibles are definitely the way to expand beyond two POD arguments. At some point with TracingV2 it that might evolve into protos, but it's still the way to go for now.

@oysteine: wouldn't such a disabled-by-default event result in an extra "call" layer in the trace though? Seems unfortunate that every scheduler task would have two layers before getting into the real task itself.

Would there instead be a way to have disabled-by-default extra args on the associated TaskSchedulerRunTask toplevel event?

You could do that by using a convertable for the args. In the code that sets the values into the convertable it always sets the src_file and src_func. You then check if the disabled-by-default category is enabled, and if so, dump the rest of your information into the convertable.

See https://cs.chromium.org/chromium/src/cc/trees/layer_tree_host_impl.cc?q=CATEGORY_GROUP_ENABLED+cc.debug&sq=package:chromium&l=678&dr=C

#7: That's a pretty cool idea!

+charliea and nednguyen who's been looking at exactly the trace size bloat due to toplevel tasks lately. I'm wondering if we could move src_file and src_func as well.

@#8, I'm working on a change to make base::PendingTask implement ConvertableToTraceFormat so that it only takes a single arg slot instead of both. I'll make it print under the same conditions it did before but you could easily adapt it not to if you want later (though I do believe there's a lot of crucial information in the toplevel category as it stands today).

So to make sure I understand the proposal, we only add the args for top_level trace events if "disabled-by-default" category is enabled?

That seems good to me. 

+benjhayden: Are you aware of any hard dependencies in Catapult on the src_file and src_func args for task events?

So if I want |task| in INTERNAL_TRACE_TASK_EXECUTION to only use one argument of AddTraceEvent (so that more advanced tasks -- e.g. scheduler's) can tack on a second argument, I should use ConvertableToTraceFormat.

But ConvertableToTraceFormat has to be handed over via unique_ptr FWIU which isn't possible for base::PendingTask.

It seems weird to me that handing off ownership is required, I guess I could have base::PendingTask farm out independent objects which can be handed to tracing?

What's the recommended approach here?

Thanks,
Gab

Yes, an object with its own lifespan would be needed. The calling code doesn't know when and on which thread the object will finally serialized (if ever, for ring-buffer mode).

+ssid who added some of these macros for memory-infra and knows about their usage.

Not sure if it's relevant: There is a discussion about moving these macros out of trace_event.h into different file, since it causes dependency to heap_profiler. Maybe these should be moved to task_scheduler, and I would be happier if SequencedWorkerPool::Inner::ThreadLoop also uses the same macro / similar one with flow if possible, in case all the other places add more args.

I don't think base/task_scheduler is the right place for those macros, there are many more places in Chrome today that need to trace toplevel task posting.

SequencedWorkerPool::Inner::ThreadLoop is likely going away in favor of TaskScheduler in the medium term so I wouldn't put too much effort there.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4d92485f89ba1de9d4e4f0f37789a0055e19daae

commit 4d92485f89ba1de9d4e4f0f37789a0055e19daae
Author: gab <gab@chromium.org>
Date: Mon Sep 26 21:00:45 2016

Remove unneeded thread.h includes and fix IWUU.

Include removed from trace_event_impl.h and leveldb_mojo_proxy.h

Removing unnecessary thread.h includes allows including trace_event_impl.h
in pending_task.h without introducing a cyclic dependency
(in a follow-up CL).

BUG= 649084 
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

TBR=wez,pastarmovj,rockot,sadrul (ref. https://codereview.chromium.org/2365623003/#msg68)

Review-Url: https://codereview.chromium.org/2365623003
Cr-Commit-Position: refs/heads/master@{#420989}

[modify] https://crrev.com/4d92485f89ba1de9d4e4f0f37789a0055e19daae/base/threading/sequenced_worker_pool.cc
[modify] https://crrev.com/4d92485f89ba1de9d4e4f0f37789a0055e19daae/base/trace_event/trace_event_android.cc
[modify] https://crrev.com/4d92485f89ba1de9d4e4f0f37789a0055e19daae/base/trace_event/trace_event_impl.h
[modify] https://crrev.com/4d92485f89ba1de9d4e4f0f37789a0055e19daae/base/trace_event/trace_log.cc
[modify] https://crrev.com/4d92485f89ba1de9d4e4f0f37789a0055e19daae/blimp/client/core/compositor/blimp_compositor_frame_sink_unittest.cc
[modify] https://crrev.com/4d92485f89ba1de9d4e4f0f37789a0055e19daae/blimp/client/core/compositor/blimp_compositor_unittest.cc
[modify] https://crrev.com/4d92485f89ba1de9d4e4f0f37789a0055e19daae/cc/raster/raster_buffer_provider_unittest.cc
[modify] https://crrev.com/4d92485f89ba1de9d4e4f0f37789a0055e19daae/cc/test/fake_external_begin_frame_source.h
[modify] https://crrev.com/4d92485f89ba1de9d4e4f0f37789a0055e19daae/cc/tiles/picture_layer_tiling_set.h
[modify] https://crrev.com/4d92485f89ba1de9d4e4f0f37789a0055e19daae/cc/trees/remote_channel_impl.h
[modify] https://crrev.com/4d92485f89ba1de9d4e4f0f37789a0055e19daae/chrome/browser/safe_browsing/protocol_manager_unittest.cc
[modify] https://crrev.com/4d92485f89ba1de9d4e4f0f37789a0055e19daae/components/leveldb/leveldb_mojo_proxy.cc
[modify] https://crrev.com/4d92485f89ba1de9d4e4f0f37789a0055e19daae/components/leveldb/leveldb_mojo_proxy.h
[modify] https://crrev.com/4d92485f89ba1de9d4e4f0f37789a0055e19daae/components/policy/core/common/cloud/policy_header_io_helper_unittest.cc
[modify] https://crrev.com/4d92485f89ba1de9d4e4f0f37789a0055e19daae/components/policy/core/common/cloud/policy_header_service_unittest.cc
[modify] https://crrev.com/4d92485f89ba1de9d4e4f0f37789a0055e19daae/content/browser/gpu/gpu_process_host.h
[modify] https://crrev.com/4d92485f89ba1de9d4e4f0f37789a0055e19daae/content/browser/renderer_host/render_message_filter.cc
[modify] https://crrev.com/4d92485f89ba1de9d4e4f0f37789a0055e19daae/content/browser/renderer_host/render_message_filter.h
[modify] https://crrev.com/4d92485f89ba1de9d4e4f0f37789a0055e19daae/content/browser/tracing/background_tracing_config_unittest.cc
[modify] https://crrev.com/4d92485f89ba1de9d4e4f0f37789a0055e19daae/content/renderer/media/audio_renderer_sink_cache_unittest.cc
[modify] https://crrev.com/4d92485f89ba1de9d4e4f0f37789a0055e19daae/extensions/browser/api/display_source/display_source_apitestbase.cc
[modify] https://crrev.com/4d92485f89ba1de9d4e4f0f37789a0055e19daae/ipc/attachment_broker_mac_unittest.cc
[modify] https://crrev.com/4d92485f89ba1de9d4e4f0f37789a0055e19daae/ipc/ipc_send_fds_test.cc
[modify] https://crrev.com/4d92485f89ba1de9d4e4f0f37789a0055e19daae/ipc/ipc_test_channel_listener.cc
[modify] https://crrev.com/4d92485f89ba1de9d4e4f0f37789a0055e19daae/media/cast/test/end2end_unittest.cc
[modify] https://crrev.com/4d92485f89ba1de9d4e4f0f37789a0055e19daae/media/gpu/android_video_decode_accelerator.cc
[modify] https://crrev.com/4d92485f89ba1de9d4e4f0f37789a0055e19daae/third_party/WebKit/Source/web/tests/DEPS
[modify] https://crrev.com/4d92485f89ba1de9d4e4f0f37789a0055e19daae/third_party/WebKit/Source/web/tests/TimerPerfTest.cpp
[modify] https://crrev.com/4d92485f89ba1de9d4e4f0f37789a0055e19daae/third_party/WebKit/Source/web/tests/VirtualTimeTest.cpp
[modify] https://crrev.com/4d92485f89ba1de9d4e4f0f37789a0055e19daae/ui/events/ozone/device/udev/device_manager_udev.cc
[modify] https://crrev.com/4d92485f89ba1de9d4e4f0f37789a0055e19daae/ui/gl/gl_context_cgl.cc
[modify] https://crrev.com/4d92485f89ba1de9d4e4f0f37789a0055e19daae/ui/ozone/public/ozone_gpu_test_helper.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cf3416047e29d074bdc135488236bd72cbd7f97b

commit cf3416047e29d074bdc135488236bd72cbd7f97b
Author: gab <gab@chromium.org>
Date: Mon Sep 26 23:42:27 2016

Suppress definition of snprintf in port_chromium.h which conflicts with base::snprintf.

This definition breaks compile whenever a compilation unit happens to include string_util.h
before port_chromium.h (as the defined base::snprintf() doesn't match the callers
base::_snprintf() calls as replaced by the preprocessor)... This happened to work
before when string_util.h was included after as the definition would also come out as
base::_snprintf() thanks the preprocessor..!

The definition doesn't appear to be needed by anything but env_chromium.cc which can use
base::snprintf instead.

This was discovered while adding a new header to pending_task.h which made env_mojo.cc
hit a compile error:

src\third_party\leveldatabase\chromium_logger.h(46): error C2039: '_snprintf': is not a member of 'base'
src\base\trace_event\trace_event.h(1137): note: see declaration of 'base'

after much digging, the issue was discovered to be that the new header in pending_task.h transitively
caused env_mojo.cc to include string_util.h before port_chromium.h and triggered this issue.

BUG= 649084 

Review-Url: https://codereview.chromium.org/2364903003
Cr-Commit-Position: refs/heads/master@{#421040}

[modify] https://crrev.com/cf3416047e29d074bdc135488236bd72cbd7f97b/third_party/leveldatabase/env_chromium.cc
[modify] https://crrev.com/cf3416047e29d074bdc135488236bd72cbd7f97b/third_party/leveldatabase/port/port_chromium.h

Sorry I missed this. It looks like the only place that src_func is really used is slice_title_fixer, which is a UI nice-to-have:
https://github.com/catapult-project/catapult/search?utf8=✓&q=src_func
src_file doesn't appear used at all.
However, I have found these fields useful when manually reading traces.
Were they removed or changed?

#19: The context is that there was discussion here about adding additional args (which requires a convertible), but as "toplevel" is already a big source of trace file bloat we were looking into making the args essentially optional (through a disabled-by-default category check).

Do you mean the fields are useful when manually reading your own test traces, or traces produced by the waterfall?

It's possible that we would want to manually read a waterfall trace, and in that case, src_file/src_func might be helpful, but I think that should be rare enough that you can move them to a disabled-by-default category, and we can add that disabled-by-default category to a benchmark if+when it becomes necessary to diagnose a specific regression. SG?

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b7555076c5684f2679c0f63be683cde91cf6e215

commit b7555076c5684f2679c0f63be683cde91cf6e215
Author: gab <gab@chromium.org>
Date: Thu Oct 06 21:16:24 2016

Add a task_scheduler tracing category which will record an extra event per task.

That event will contain ExecutionMode/SequenceToken and TaskPriority.

It would be better to tack an optional extra arg to the main task event but
tracing doesn't currently allow this (http://crbug.com/652692).

BUG= 649084 

Review-Url: https://codereview.chromium.org/2392903002
Cr-Commit-Position: refs/heads/master@{#423681}

[modify] https://crrev.com/b7555076c5684f2679c0f63be683cde91cf6e215/base/sequence_token.cc
[modify] https://crrev.com/b7555076c5684f2679c0f63be683cde91cf6e215/base/sequence_token.h
[modify] https://crrev.com/b7555076c5684f2679c0f63be683cde91cf6e215/base/sequence_token_unittest.cc
[modify] https://crrev.com/b7555076c5684f2679c0f63be683cde91cf6e215/base/task_scheduler/task_tracker.cc
[modify] https://crrev.com/b7555076c5684f2679c0f63be683cde91cf6e215/base/task_scheduler/task_traits.cc
[modify] https://crrev.com/b7555076c5684f2679c0f63be683cde91cf6e215/base/task_scheduler/task_traits.h

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b7555076c5684f2679c0f63be683cde91cf6e215

commit b7555076c5684f2679c0f63be683cde91cf6e215
Author: gab <gab@chromium.org>
Date: Thu Oct 06 21:16:24 2016

Add a task_scheduler tracing category which will record an extra event per task.

That event will contain ExecutionMode/SequenceToken and TaskPriority.

It would be better to tack an optional extra arg to the main task event but
tracing doesn't currently allow this (http://crbug.com/652692).

BUG= 649084 

Review-Url: https://codereview.chromium.org/2392903002
Cr-Commit-Position: refs/heads/master@{#423681}

[modify] https://crrev.com/b7555076c5684f2679c0f63be683cde91cf6e215/base/sequence_token.cc
[modify] https://crrev.com/b7555076c5684f2679c0f63be683cde91cf6e215/base/sequence_token.h
[modify] https://crrev.com/b7555076c5684f2679c0f63be683cde91cf6e215/base/sequence_token_unittest.cc
[modify] https://crrev.com/b7555076c5684f2679c0f63be683cde91cf6e215/base/task_scheduler/task_tracker.cc
[modify] https://crrev.com/b7555076c5684f2679c0f63be683cde91cf6e215/base/task_scheduler/task_traits.cc
[modify] https://crrev.com/b7555076c5684f2679c0f63be683cde91cf6e215/base/task_scheduler/task_traits.h

[Automated comment] removing mislabelled merge-merged-2840