Creating a framework for suppressing pollution of the profiler data

base/threading/sequenced_worker_pool.cc

Index: base/threading/sequenced_worker_pool.cc
diff --git a/base/threading/sequenced_worker_pool.cc b/base/threading/sequenced_worker_pool.cc
index 532f26ebe5c784eba7613045ef99f5c40b012714..9e51d645894dafe379a6dc3c244ab0b42d8f2b87 100644
--- a/base/threading/sequenced_worker_pool.cc
+++ b/base/threading/sequenced_worker_pool.cc
@@ -755,13 +755,14 @@ void SequencedWorkerPool::Inner::ThreadLoop(Worker* this_worker) {
           this_worker->set_running_task_info(
               SequenceToken(task.sequence_token_id), task.shutdown_behavior);
 
-          tracked_objects::TrackedTime start_time =
-              tracked_objects::ThreadData::NowForStartOfRun(task.birth_tally);
-
+          tracked_objects::TaskStopwatch stopwatch;
+          stopwatch.Start(
+              tracked_objects::ThreadData::NowForStartOfRun(task.birth_tally));

[jar (doing other things), 2014/09/02 16:52:16]

The reason there is an API called NowForStartOfRun(), instead of just calling
tracked_objects::Now() (a special flavor of Now()) is to make the API concise
(one call, induces side effects, and gets the time value that we need).  We
could have used two function calls... but that would be less concise, an error
prone.

That is indeed also why we have NowForEndOfRun() (gets time, and induces side
effect (of which there are currently none!)).  

That also allowed us to sandwich as tightly as possible the acquisition of the
times around the Run() event we wanted to time.


Given that the StopWatch class is for the exclusive use of the profiler, it can
acquire scoped_objects::Now() directly (no need to even pass in timer values). 
It can always get them as needed from the Now() function.

As a result, the above lines should probably read:
tracked_objects::ThreadData::PrepareForStartOfRun(task.birth_tally);
tracked_objects::TaskStopwatch stopwatch; // Implicitly starts timer.

[vadimt, 2014/09/02 22:41:37]

Done.

           task.task.Run();
+          stopwatch.Stop(tracked_objects::ThreadData::NowForEndOfRun());

[jar (doing other things), 2014/09/02 16:52:16]

This call can then be simply:
stopwatch.Stop();

It may even be that we don't need to call this, and the first line of the next
method *could* be the call to Stop().

In general, it is nicest to tightly sandwidth the Run() with the start (via
construction?) and the Stop (via explicit call). 

[vadimt, 2014/09/02 22:41:37]

Done.
Given that there are 3 methods that can follow stopwatch.Stop
(TallyRunOnNamedThreadIfTracking, TallyRunOnWorkerThreadIfTracking and
TallyRunInAScopedRegionIfTracking), it's not elegant to call Stop in all 3 of
them. So I kept a separate Stop method.

 
-          tracked_objects::ThreadData::TallyRunOnNamedThreadIfTracking(task,
-              start_time, tracked_objects::ThreadData::NowForEndOfRun());
+          tracked_objects::ThreadData::TallyRunOnNamedThreadIfTracking(
+              task, stopwatch);

[jar (doing other things), 2014/09/02 16:52:16]

If you're willing to embed the Stop() into the above call, we'd instead pass in:
&stopwatch.

The big question is to look at all call sites, and see if there are other
calculations done during the call (as args are passed in).  It would be best to
do the stop very shortly after the task completes.  

[vadimt, 2014/09/02 22:41:36]

See the above answer.

 
           // Make sure our task is erased outside the lock for the
           // same reason we do this with delete_these_oustide_lock.