Support profiling of tasks run as sequenced_tasks

base/threading/sequenced_worker_pool.cc

Index: base/threading/sequenced_worker_pool.cc
===================================================================
--- base/threading/sequenced_worker_pool.cc	(revision 148495)
+++ base/threading/sequenced_worker_pool.cc	(working copy)
@@ -35,16 +35,21 @@
 
 namespace {
 
-struct SequencedTask {
+struct SequencedTask : public TrackingInfo  {

[akalin, 2012/07/27 17:45:39]

any particular reason for using inheritance?  Why not just have a TrackingInfo
member?

[jar (doing other things), 2012/07/27 18:15:46]

First note that TrackingInfo is also a struct.  

Truth be known... I used inherritance to continue matching the code structure
used in message_loop.cc as precisely as possible.  

I only recently learned that we "prefer" encapsulation via a member, to
inheritance.  In this case, the encapsulation would just imply that references
to the struct member of the encapsulated member would be a bit more verbose (and
we'd call into the profiler passing only the tracking info, rather than the
"task").

I can change it here (and even in the corresponding code in message_loop as
well) if you significantly prefer it.  I could also do such a change (to both
call sites) as a separable CL.

On 2012/07/27 17:45:39, akalin wrote:

[akalin, 2012/07/27 20:18:18]

Doing it as a separate CL sgtm.

   SequencedTask()
       : sequence_token_id(0),
         shutdown_behavior(SequencedWorkerPool::BLOCK_SHUTDOWN) {}
 
+  explicit SequencedTask(const tracked_objects::Location& from_here)
+      : base::TrackingInfo(from_here, TimeTicks()),
+        sequence_token_id(0),
+        shutdown_behavior(SequencedWorkerPool::BLOCK_SHUTDOWN) {}
+
   ~SequencedTask() {}
 
   int sequence_token_id;
   SequencedWorkerPool::WorkerShutdown shutdown_behavior;
-  tracked_objects::Location location;
+  tracked_objects::Location posted_from;

[akalin, 2012/07/27 17:45:39]

what's the difference between this and time_posted (from TrackingInfo)?  I don't
see this used anywhere...

[jar (doing other things), 2012/07/27 18:15:46]

This change was a name change only (in this file), to a more descriptive name,
that also matched the usage in message_loop and worker_pool.

The (historic) value of this field tended to appear when running a debugger, and
allows a developer to see where the task was "posted_from."

I think it is the case that *now* that the profiler is indeed always on (it
currently includes code to turn off profiling, either statically, or
dynamically), that we can deduce this location by traversing into the profiler
data structures.

I probably should create a bug to rip out the conditional code in profiler, and
then revisit this placement of the field into the tasks.

That change seems pretty orthogonal to this... which just improved the name (but
it also made me notice the variable wasn't getting much use, and might not be
needed anymore, even for developers).

On 2012/07/27 17:45:39, akalin wrote:

[jbates, 2012/07/27 19:54:10]

Btw, I'll use this posted_from for tracing these tasks with the TRACE_EVENT
patch. The Location is not actually retained by TrackingInfo, although it could
be.

[akalin, 2012/07/27 20:18:18]

I'm okay with ripping it out in a separate CL if needed, if it's orthogonal.

[jar (doing other things), 2012/07/27 21:52:29]

Although task doesn't appear to directly save it... it links to the location
(most of the time).

task.birth_tally is a class Births
Today is is NULL if tracking is disabled... but would always be defined if we
always did profiling.

Then
task.birth_tally.Location()
would get you the actual posted_from value.

Today, due to the possible NULL Births*, it might not always work... but if I
ensured it was always set, then you'd be good to go.  It is a bit more complex
for folks to see the data... but it really doesn't need to be here (I think).

Note that a location instance is merely an int, plus a pointer to an atom of a
string, so the memory implications of the current approach is not significant.

...also... if I stopped doing this via inheritance, and instead used
encapsulation, it would be more like:

task.tracking_info.birth_tally.Location()

   Closure task;
 };
 
@@ -481,10 +486,10 @@
     WorkerShutdown shutdown_behavior,
     const tracked_objects::Location& from_here,
     const Closure& task) {
-  SequencedTask sequenced;
+  SequencedTask sequenced(from_here);
   sequenced.sequence_token_id = sequence_token.id_;
   sequenced.shutdown_behavior = shutdown_behavior;
-  sequenced.location = from_here;
+  sequenced.posted_from = from_here;
   sequenced.task = task;
 
   int create_thread_id = 0;
@@ -614,8 +619,14 @@
           this_worker->set_running_sequence(
               SequenceToken(task.sequence_token_id));
 
+          tracked_objects::TrackedTime start_time =
+             tracked_objects::ThreadData::NowForStartOfRun(task.birth_tally);

[jbates, 2012/07/27 19:54:10]

nit: one more space on indent

[jar (doing other things), 2012/07/27 21:52:29]

Done.

+
           task.task.Run();
 
+          tracked_objects::ThreadData::TallyRunOnNamedThreadIfTracking(task,
+              start_time, tracked_objects::ThreadData::NowForEndOfRun());
+
           this_worker->set_running_sequence(SequenceToken());
 
           // Make sure our task is erased outside the lock for the same reason