histogram synchronizer code between renderer and browser for UMA and about_histogram

Hi Jim,
  When you have sometime, could you look at this diff?

thanks,
raman

[raman, 2009-03-23 06:59:43]

Hi Jim,
  I have uploaded the patch we have been working together. Would appreciate your
comments.

thanks very much,
raman

[jar (doing other things), 2009-03-26 02:21:22]

This is looking very good.

I made some suggestions about moving code around, and I think I noticed a bug.

Please see if you can make sense of my comments.

I was a bit rushed... and need to look at it some more, but hopefully there are
several items you can think about.

Thanks for all this effort!!!

Jim

http://codereview.chromium.org/42496/diff/1020/27
File chrome/browser/browser_about_handler.cc (right):

http://codereview.chromium.org/42496/diff/1020/27#newcode147
Line 147: }
I think the above code (lines 142-147) could be pushed into the
WatiForRendererHistograms() call below.  All that functionality is really the
business of the HistogramSynchronizer I think.

http://codereview.chromium.org/42496/diff/1020/22
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/42496/diff/1020/22#newcode895
Line 895: void MetricsService::TryToStartTransmission() {
Perhaps we can modularize a bit, and have the "flow" controlled strictly from
this class?  
I'd suggest changing this method name to:
void MetricsService::OnHistogramSynchronizationDone()

You could then add one more intermediate method with a name:

void MetricsService::OnMemoryDetailCollectionDone()

which posts a task to start the HistogramSynchronization()

This way the code to gather memory details won't need to know about histogram
gathering... and we can add other intermediate tasks as needed... or change
orders... etc.

http://codereview.chromium.org/42496/diff/1020/22#newcode1062
Line 1062: &MetricsService::TryToStartTransmission);
Per the modularization suggestion above, you could move 
CollectRenderHistograms() to the HistogramSyncrhonizer class.  You could then
instead call it with an argument which is the callback_task you just created
(line 1062).  This way MetricsService knows the names of the callback tasks...
and this method (in HistogramSynchronizer) knows how to do the  synchronization
(notify all tasks, etc.).  I'd also suggest you pass in an argument telling it
how long to wait before absolutely calling us back on the task.

http://codereview.chromium.org/42496/diff/1020/22#newcode1080
Line 1080: timer_pending_ = true;
You won't need to set timer_pending_ here.

http://codereview.chromium.org/42496/diff/1020/22#newcode1085
Line 1085: NewRunnableMethod(&MetricsService::ForceHistogramTransmission),
This code to PostDelayedTask() should be hidden in the HistogramSynchronizer
class I think.

http://codereview.chromium.org/42496/diff/1020/22#newcode1089
Line 1089: void MetricsService::ForceHistogramTransmission() {
This method should also be hidden in the HistogramSynchronizer class.  That
class may have a member which is the calback to use... and it may have an
assertion that the task should be null when first called... and then it nulls it
out after it calls back.  That way you won't need to know about the
timer_pending_ variable (in a sense, you'll maintain state to show that you are
handling your section of the timer_pending_ interval...  as you'll have a
callback handy at that time... but you don't need to see into this class to do
your job... and you do need to manipulate the state a bit more.  

As coded here, we don't really terminate when the wait time expires (I think),
we just start anther set of renderer notifications, and we never "give up."  To
illuminate this bug, you should run chrome, attached to a child process, and
break when you're asked to update histograms.  You'll see (I think) that the
browser will try again... and again... and again... to get all the renderers to
call back.

http://codereview.chromium.org/42496/diff/1020/28
File chrome/common/histogram_synchronizer.cc (right):

http://codereview.chromium.org/42496/diff/1020/28#newcode23
Line 23: received_all_renderer_historgrams_ = new ConditionVariable(lock_);
Both of the above could probably go into constructor initialization lists.

http://codereview.chromium.org/42496/diff/1020/30
File chrome/common/histogram_synchronizer.h (right):

http://codereview.chromium.org/42496/diff/1020/30#newcode61
Line 61: static Task* callback_task_;
Since you'll only have one of these classes, I'd suggest that a lot of these
statics be made non-static member variables.  That you can also control when you
discard them more nicely.  You might also be able to make this Task* a
scoped_refptr<Task> so that deletion is handled automatically.

You can then have singel static member that points to *the* solo instance of
this class, which is created when you call the initialization routine.

http://codereview.chromium.org/42496/diff/1020/30#newcode64
Line 64: static ConditionVariable* received_all_renderer_historgrams_;
Move this down to after the declaration of lock.  It is good to keep the items
order to match what the way you initialize them.  (I suggested putting these
items in the initializer list... and that will *require* that you have them in
the order of first a lock, and then a cv).

[raman, 2009-04-06 06:16:10]

Hi jim,
  I have made all the changes you have suggested (except the callback to be
called when wait time expires. When you have some time, I would like to go over
what I had done. 

  I am recomipling everything. I might haev made some minstakes while making all
static data into member variables (will debug tonight/tomorrow).

thanks,
raman

http://codereview.chromium.org/42496/diff/1020/27
File chrome/browser/browser_about_handler.cc (right):

http://codereview.chromium.org/42496/diff/1020/27#newcode147
Line 147: }
On 2009/03/26 02:21:23, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/1020/22
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/42496/diff/1020/22#newcode895
Line 895: void MetricsService::TryToStartTransmission() {
On 2009/03/26 02:21:23, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/1020/22#newcode1062
Line 1062: &MetricsService::TryToStartTransmission);
On 2009/03/26 02:21:23, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/1020/22#newcode1080
Line 1080: timer_pending_ = true;
On 2009/03/26 02:21:23, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/1020/22#newcode1085
Line 1085: NewRunnableMethod(&MetricsService::ForceHistogramTransmission),
On 2009/03/26 02:21:23, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/1020/22#newcode1089
Line 1089: void MetricsService::ForceHistogramTransmission() {
On 2009/03/26 02:21:23, jar wrote:

Hi Jim,
  I have made the changes. We call the callback task
(OnHistogramSynchronizationDone) once the wait time elpases and if there is a
callback task exists. We clear the callback_task immediately after calling the
callback_task. Can we discuss when we meet. 

thanks,
raman

http://codereview.chromium.org/42496/diff/1020/28
File chrome/common/histogram_synchronizer.cc (right):

http://codereview.chromium.org/42496/diff/1020/28#newcode23
Line 23: received_all_renderer_historgrams_ = new ConditionVariable(lock_);
On 2009/03/26 02:21:23, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/1020/30
File chrome/common/histogram_synchronizer.h (right):

http://codereview.chromium.org/42496/diff/1020/30#newcode61
Line 61: static Task* callback_task_;
On 2009/03/26 02:21:23, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/1020/30#newcode64
Line 64: static ConditionVariable* received_all_renderer_historgrams_;
On 2009/03/26 02:21:23, jar wrote:

Done.

[jar (doing other things), 2009-04-07 06:21:26]

I looked basically at the delta so that I could be sure to comment quickly.  (I
felt embarrassed that I took so long on the last review).

I think you resolved the correctness issue completely, and I just had some
additional questions about various pieces (lock vs use of the IO thread to get
thread safety).  

I also tried to look at some of the names you were using for members, and you
might try to use the pattern to help with the method names as well.

IMO, getting a feel for when to use locks vs when to use thread based isolation
is one of the hardest parts of this architecture.  In this case, you <sarcasm>
have the pleasure </sarcasm> of using both in one class (which is rather
unusual... but needed here).  As a result, some of the code is a little strange,
but all in all, it should be a pretty educational adventure to use each
protection mechanism in the "right" place.

I hope the comments are readable, and not to harsh.  I know you're spending a
lot of time on this, and at the same time, doing a LOT of other stuff for your
real job and your family.  Please don't let this damage either of those
activities.

Thanks,

Jim

p.s., Please don't be depressed that this is going back and forth a few times. 
A coworker recently reminded me how depressing this can be for a noogler, which
brought back my own personal memories.   I hope your enjoying this enough that
you can put up with the progression.  It really does end up producing a better
product.

http://codereview.chromium.org/42496/diff/4001/5003
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/42496/diff/4001/5003#newcode923
Line 923: }
I think this method should do little more than create a callback task (as you do
on line 904), decided how long we're willing to wait (as you do on line 907),
and then make a call (probably via a post-task) to the histogram synchronizer
with those two arguments. Everything else should be hidden in that class.

http://codereview.chromium.org/42496/diff/4001/5004
File chrome/browser/metrics/metrics_service.h (right):

http://codereview.chromium.org/42496/diff/4001/5004#newcode199
Line 199: void MetricsService::OnMemoryDetailCollectionDone();
Linux build will complain about "MetricsService::" here in the class
declaration, so you'll need to remove it.

Please put callback declarations currently on line 199 and 203 next to each
other, and extend the comment on line 201 to say "Do not call either
OnHistogramSynchronizationDone() or OnMemoryDetailCollectionDone() directly."

http://codereview.chromium.org/42496/diff/4001/5009
File chrome/common/histogram_synchronizer.cc (right):

http://codereview.chromium.org/42496/diff/4001/5009#newcode51
Line 51: histogram_synchronizer_ = histogram_synchronizer;
You have to be sure to think through which thread sets and resets this pointer. 
This may all work out as it is currently defined, as it is initialized on the UI
thread, and presumably destroyed after all the other threads have gone away.  As
a result, it is ok to at least call it from the UI thread (for UMA uploads), or
for about:histograms.

Perhaps a comment in the header would help other folks be assured of this as
well.

http://codereview.chromium.org/42496/diff/4001/5009#newcode59
Line 59: void HistogramSynchronizer::WaitForRendererHistograms() {
Add a comment to note that his is (I think) the only method in this class called
on a thread other than the IO thread.  You might even mention that it is called
on the main UI thread.

http://codereview.chromium.org/42496/diff/4001/5009#newcode92
Line 92: AutoLock auto_lock(*lock_);
Instead of using a lock for this methed...
Perhaps you call this *only* on the IO thread, and use PostTask() to call it
(with the callback as an argument).

http://codereview.chromium.org/42496/diff/4001/5009#newcode94
Line 94: // Asser there was no callback_task_ already.
nit: typo: 
Asser --> Assert

You might even delete the comment, as the assert is pretty clear.

http://codereview.chromium.org/42496/diff/4001/5009#newcode102
Line 102: it->second->Send(new ViewMsg_GetRendererHistograms(sequence_number));
I always try to hold a lock for only the critical data items.  Do we need the
lock held while we call Send()?  If not, can we do the entire for loop outside
of the locked region?
As noted above, perhaps you can avoid holding the lock completely, by making
sure you're on teh IO thread.  I *think* you may need to be on the IO thread to
iterate over the renderer processes.  ...or am I mitaken, and you have to be on
the UI thread?

http://codereview.chromium.org/42496/diff/4001/5009#newcode136
Line 136: if ((about_renderer_histograms_requested_ == 0) ||
You asked me to comment on some long names...
This name is correct, but perchance the better distinction to note is that
"about" is doing "synchronous" waiting, while "uma" is doing "asynchronous" or
"callback" or "tasked" waiting.  The word "histogram" in the name is probably
redundant, given the class name.

So perhaps a better name might be:
synchronous_request_count_  (which induces a signal)
vs
async_request_count_ (which posts a callback)

The sequence numbers might then be:
synchronous_sequence_number_
vs
async_sequence_number_

http://codereview.chromium.org/42496/diff/4001/5009#newcode139
Line 139: received_all_renderer_historgrams_->Signal();
This name is long... but I think it is excellent.

http://codereview.chromium.org/42496/diff/4001/5009#newcode141
Line 141: } else if (sequence_number == uma_renderer_histogram_sequence_number_)
{
Again here, I think it is better to hold the lock for the smallest period of
time.  In this case, you only need to hold the lock to check on the
about_render_histogram_requested_ (and to do the Signal()).
If you first tried to match the UMA sequence number.. and if  you matched, you
eventually did a return.... then you could next acquire the lock only for the
about case.

http://codereview.chromium.org/42496/diff/4001/5009#newcode155
Line 155: AutoLock auto_lock(*lock_);
You probably don't need the lock here (assuming you rely mostly on teh IO thread
to restrict access to the uma related members.

http://codereview.chromium.org/42496/diff/4001/5011
File chrome/common/histogram_synchronizer.h (right):

http://codereview.chromium.org/42496/diff/4001/5011#newcode54
Line 54: // sequence number. Returns true when we receivie histograms from all
typo: receivie --> receive

http://codereview.chromium.org/42496/diff/4001/5011#newcode66
Line 66: // lock protects access to the above map and sequence number.
I didn't see a map.  Perhaps this was a typo.

I also think (see other comments) that the lock will only be  significant for
the about related members (sequence number and count of requested renderers)

http://codereview.chromium.org/42496/diff/4001/5011#newcode67
Line 67: Lock* lock_;
You may as well make this an actual Lock, then you won't have to new one up, and
delete it.  If you really wanted a pointer, since we own the lock (and have to
delete it), you could use a smart pointer.

I think defining the member to be the lock would be better.

http://codereview.chromium.org/42496/diff/4001/5011#newcode69
Line 69: ConditionVariable* received_all_renderer_historgrams_;
You can also make this a real CV, rather than a pointer.  So long as the Lock is
declared first (and hence initialized first), you can put that "&lock_" as the
initializer for the class constructor.  This way you won't have to delete the CV
manually in the class destructor.

[raman, 2009-04-12 06:49:30]

Hi Jim,
  Made the changes you had suggested in the review. Could you please take a look
at the diffs when you have sometime.

thanks much,
raman

http://codereview.chromium.org/42496/diff/4001/5003
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/42496/diff/4001/5003#newcode923
Line 923: }
On 2009/04/07 06:21:26, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/4001/5004
File chrome/browser/metrics/metrics_service.h (right):

http://codereview.chromium.org/42496/diff/4001/5004#newcode199
Line 199: void MetricsService::OnMemoryDetailCollectionDone();
On 2009/04/07 06:21:26, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/4001/5009
File chrome/common/histogram_synchronizer.cc (right):

http://codereview.chromium.org/42496/diff/4001/5009#newcode51
Line 51: histogram_synchronizer_ = histogram_synchronizer;
On 2009/04/07 06:21:26, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/4001/5009#newcode59
Line 59: void HistogramSynchronizer::WaitForRendererHistograms() {
On 2009/04/07 06:21:26, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/4001/5009#newcode92
Line 92: AutoLock auto_lock(*lock_);
On 2009/04/07 06:21:26, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/4001/5009#newcode94
Line 94: // Asser there was no callback_task_ already.
On 2009/04/07 06:21:26, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/4001/5009#newcode102
Line 102: it->second->Send(new ViewMsg_GetRendererHistograms(sequence_number));
On 2009/04/07 06:21:26, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/4001/5009#newcode136
Line 136: if ((about_renderer_histograms_requested_ == 0) ||
On 2009/04/07 06:21:26, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/4001/5009#newcode139
Line 139: received_all_renderer_historgrams_->Signal();
On 2009/04/07 06:21:26, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/4001/5009#newcode141
Line 141: } else if (sequence_number == uma_renderer_histogram_sequence_number_)
{
On 2009/04/07 06:21:26, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/4001/5009#newcode155
Line 155: AutoLock auto_lock(*lock_);
On 2009/04/07 06:21:26, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/4001/5011
File chrome/common/histogram_synchronizer.h (right):

http://codereview.chromium.org/42496/diff/4001/5011#newcode54
Line 54: // sequence number. Returns true when we receivie histograms from all
On 2009/04/07 06:21:26, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/4001/5011#newcode66
Line 66: // lock protects access to the above map and sequence number.
On 2009/04/07 06:21:26, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/4001/5011#newcode67
Line 67: Lock* lock_;
On 2009/04/07 06:21:26, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/4001/5011#newcode69
Line 69: ConditionVariable* received_all_renderer_historgrams_;
On 2009/04/07 06:21:26, jar wrote:

Done.

[raman, 2009-04-12 19:59:21]

Hi Jim,
  It is same as lastnight's patch. Added DCHECK for io_thread.

[jar (doing other things), 2009-04-18 18:36:00]

I need to carefully read the two new files again.  I'm sorry I ran out of time
this week and didn't get to it.

http://codereview.chromium.org/42496/diff/7030/8021
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/42496/diff/7030/8021#newcode903
Line 903: // heard from all renderers, then it will call
OnHistogramSynchronizationDone.
nit: missing close paren

http://codereview.chromium.org/42496/diff/7030/8022
File chrome/browser/metrics/metrics_service.h (right):

http://codereview.chromium.org/42496/diff/7030/8022#newcode200
Line 200: // Do not call OnHistogramSynchronizationDone() directly.
Adjust comment to:
// Do not call OnMemoryDetailCollectionDone() or
OnHistogramSynchronizationDone() directly.

[raman, 2009-04-18 21:13:40]

Hi Jim,
  Made the changes you have suggested. Made couple of methods private. It looks
like NewRunnableMethod may not be thread safe and needs to called on current
messageloop (we post all the tasks on IO thread, backed off making couple of
methods non-static methods). Would like to run it by you when you sometime.

thanks,
raman

http://codereview.chromium.org/42496/diff/7030/8021
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/42496/diff/7030/8021#newcode903
Line 903: // heard from all renderers, then it will call
OnHistogramSynchronizationDone.
On 2009/04/18 18:36:00, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/7030/8022
File chrome/browser/metrics/metrics_service.h (right):

http://codereview.chromium.org/42496/diff/7030/8022#newcode200
Line 200: // Do not call OnHistogramSynchronizationDone() directly.
On 2009/04/18 18:36:00, jar wrote:

Done.

[jar (doing other things), 2009-04-19 08:17:39]

The code is looking pretty good.

The big style comment is to try to put function descriptions in the header
files, especially for any external functions.  For private methods, it is ok to
define them in the cc file.

The two big points I'd note is that the async callback timeout method needs a
sequence number, and you probably need to be a little more careful about
handling callback_tasks, and what to do with them.  In general, you need to
either call them, or delete them (we're very tidy about this on Chrome... almost
too tidy).

I also tried to provide suggestions for names that might make some of the
methods and members a little more self explanatory. See what you think.

Thanks again for all this work!

http://codereview.chromium.org/42496/diff/7030/8027
File chrome/common/histogram_synchronizer.cc (right):

http://codereview.chromium.org/42496/diff/7030/8027#newcode28
Line 28: // about:histograms.
Great comment!  This should probably be in the header file, either at the top
(talking about the class), or more likely, at the end of the class definition,
talking about the singleton which is saved in a static member.

http://codereview.chromium.org/42496/diff/7030/8027#newcode42
Line 42: histogram_synchronizer_ = this;
Please add a DCHECK to assert that histogram_synchronizer_ is NULL before the
assignment.

http://codereview.chromium.org/42496/diff/7030/8027#newcode47
Line 47: callback_task_ = NULL;
Since this is a member, and the instance is now being destroyed, we don't need
to worry too much about clearing out the pointers.  but...
Perhaps we need to delete callback_task_ on exit, since we don't post it, and it
won't run? ...or else we should post it (if it is not null)?

My guess is that we should just delete it, as we get destroyed only at teardown,
and there is little use of trying to finish up things at this late point (by
posting back the message).

http://codereview.chromium.org/42496/diff/7030/8027#newcode58
Line 58: // This method is called on the main UI thread from about:histograms.
Move the comment into the header.

http://codereview.chromium.org/42496/diff/7030/8027#newcode60
Line 60: // Asert that the current thread is UI thread.
nit: Typo: Asert->Assert
You could delete the comment, as the DCHECK seems pretty clear in its intent.

http://codereview.chromium.org/42496/diff/7030/8027#newcode87
Line 87: DHISTOGRAM_COUNTS("Histogram.RendererHistogramsNotReceivedSynchrnous",
Nit: typo: Synchrnous-->Synchronous

http://codereview.chromium.org/42496/diff/7030/8027#newcode99
Line 99: // Asert that the current thread is UI thread.
nit: typo: Asert-->Assert

http://codereview.chromium.org/42496/diff/7030/8027#newcode106
Line 106: return;
Nice!  Add comment:
// System teardown is happening.  

We also need to delete the callback_task that was supplied as an argument or
post it. I'd suggest posting it at this point.

http://codereview.chromium.org/42496/diff/7030/8027#newcode126
Line 126: HistogramSynchronizer::ForceHistogramSynchronizationDoneCallback),
I think we have to include a sequence number, so that this callback can't be
confused with other sequences.

I think using a factory and making it a new revokable task makes a lot of sense
as well.  You'll have to make the method non-static for this.

http://codereview.chromium.org/42496/diff/7030/8027#newcode143
Line 143: DCHECK(current_synchronizer->callback_task_ == NULL);
After doing the assert, you may as well test for the existence of a previous
task, and post call to post it if it exists.  We promised to post it after some
timeout... and at this point, we should just force the posting.

http://codereview.chromium.org/42496/diff/7030/8027#newcode152
Line 152: // This method is called on the IO thread. Desrializes the histograms
and
Move comment to header file.
nit: typo: Desrializes-->Deserializes

http://codereview.chromium.org/42496/diff/7030/8027#newcode157
Line 157: // Asert that the current thread is IO thread.
nit: typo Asert-->Assert

http://codereview.chromium.org/42496/diff/7030/8027#newcode175
Line 175: // waiting process or call the callback funtion. This is called on IO
Thread.
Move comment to header.
nit: typo: funtion-->function

http://codereview.chromium.org/42496/diff/7030/8027#newcode177
Line 177: // Asert that the current thread is IO thread.
nit: typo: Asert

http://codereview.chromium.org/42496/diff/7030/8027#newcode198
Line 198: // Asert that the current thread is IO thread.
nit: typo: Asert

http://codereview.chromium.org/42496/diff/7030/8029
File chrome/common/histogram_synchronizer.h (right):

http://codereview.chromium.org/42496/diff/7030/8029#newcode24
Line 24: class HistogramSynchronizer;
You shouldn't need this forward declaration, since you're about to give the
actual declaration.

http://codereview.chromium.org/42496/diff/7030/8029#newcode38
Line 38: static HistogramSynchronizer* CurrentSynchronizer();
Please add a comment:
Return pointer to the singleton instance, which is allocated and deallocated on
the main UI thread (during system startup and teardown).

http://codereview.chromium.org/42496/diff/7030/8029#newcode40
Line 40: void WaitForRendererHistograms();
Perhaps this method should take a timeout argument, so that the caller can
decide how long it is willing to wait.

Please add a comment such as:
Contact all renderers, and get them to upload to the browser any/all changes to
histograms.  Return when all changes have been acquired, or when the wait time
expires (whichever is sooner).

http://codereview.chromium.org/42496/diff/7030/8029#newcode42
Line 42: static void GetRendererHistogramsAsynchrnous(
Since "Get" often is associated with a getter or setter pair, perhaps a better
prefix might be Acquire or Fetch??  This might make the method name:
FetchRendererHistogramsAsynchronously
The synchronous variant (above) should have a very similarly formed name
(whatever you decide on).

In any case, there is a typo in the current name spelling ...Asynchrnous rather
than ...Asynchronously

Please add comment such as:
Contact all renderers, and get them to upload to the browser any/all changes to
histograms.  When all changes have been acquired, or when the wait time expires
(whichever is sooner), post the callback_task to the UI thread.  Note the
callback_task is posted exactly once.

http://codereview.chromium.org/42496/diff/7030/8029#newcode45
Line 45: static void SetCallbackTaskToCallAfterGettingRendererHistograms(
Is this actually a private method?  Shouldn't it me non-static?

http://codereview.chromium.org/42496/diff/7030/8029#newcode48
Line 48: static void ForceHistogramSynchronizationDoneCallback();
Is this private?  Is this done by a timer?  Any chance it should be a revokable
callback, and hence a non-static method?  Shouldn't it take a sequence number as
an argument?

http://codereview.chromium.org/42496/diff/7030/8029#newcode51
Line 51: void CallCallbackTaskAndResetData();
Should this be private?

http://codereview.chromium.org/42496/diff/7030/8029#newcode57
Line 57: // records that we have received the histograms from a renderer for the
given
nit: Start sentence with upper case letter.

http://codereview.chromium.org/42496/diff/7030/8029#newcode58
Line 58: // sequence number. Returns true when we receive histograms from all
Please change comment from:
...histograms from all renderers.
to
...histograms from the last of N renderers that were contacted for an update.

http://codereview.chromium.org/42496/diff/7030/8029#newcode67
Line 67: // lock protects access to sequence number and histograms requested.
Nite: start the sentence with either:
This lock protects...
or with
lock_ protects...

http://codereview.chromium.org/42496/diff/7030/8029#newcode70
Line 70: ConditionVariable received_all_renderer_historgrams_;
Add comment:
This condition variable is used to block caller of the synchronous request to
update histograms, and to signal that thread when updates are completed.

http://codereview.chromium.org/42496/diff/7030/8029#newcode72
Line 72: Task* callback_task_;
Add comment:
When a request is made to asynchronously update the histograms, we store the
task and thread we use to post a completion notification in callback_task_ and
callback_thread_.

http://codereview.chromium.org/42496/diff/7030/8029#newcode75
Line 75: // sequence number for collecting Renderer Histograms.
Consider changing comment (as this is not actually a sequence number that we're
using):
We don't track the actual renderers that are contacted for an update, only the
count of the number of renderers, and we can sometimes time-out and give up on a
"slow to respond" renderer.  We use a sequence_number to be sure a response from
a renderer is associated with the current round of requests (and not merely a
VERY belated prior response).
renderer_histogram_sequence_number_ is the next available number (used to avoid
reuse for a long time).

http://codereview.chromium.org/42496/diff/7030/8029#newcode76
Line 76: int renderer_histogram_sequence_number_;
nit: perhaps a better name is:
next_avaible_sequence_number_

http://codereview.chromium.org/42496/diff/7030/8029#newcode78
Line 78: int async_sequence_number_;
Add comment:
The sequence number used by the most recent asynchronous update request to
contact all renderers.

http://codereview.chromium.org/42496/diff/7030/8029#newcode79
Line 79: int async_histograms_requested_;
Add comment:
The number of renderers that have not yet responded to requests (as part of an
asynchronous update).

http://codereview.chromium.org/42496/diff/7030/8029#newcode81
Line 81: int synchronous_sequence_number_;
Add comment:
The sequence number used by the most recent synchronous update request to
contact all renderers.

http://codereview.chromium.org/42496/diff/7030/8029#newcode82
Line 82: int synchronous_histograms_requested_;
Add comment:
The number of renderers that have not yet responded to requests (as part of a
synchronous update).

nit: Perhaps a better name is:
synchronous_response_pending_count_
It is a bit long... but it should only appear on short lines ;-).  It is not
really about the count of histograms, so it could be:
synchronous_renderers_requested_
or
synchronous_renderers_contacted_
or
synchronous_renderers_pending_

http://codereview.chromium.org/42496/diff/7030/8029#newcode87
Line 87: static HistogramSynchronizer* histogram_synchronizer_;
Add comment:
Only one instance of this class should be constructed, and this is the pointer
to that singleton.

[raman, 2009-04-21 15:05:43]

Hi Jim,
  Many many thanks for you patience and the awesome comments. I have made most
of the changes except the static method comment. Will talk tonight.

thanks very much,
raman

http://codereview.chromium.org/42496/diff/7030/8027
File chrome/common/histogram_synchronizer.cc (right):

http://codereview.chromium.org/42496/diff/7030/8027#newcode28
Line 28: // about:histograms.
On 2009/04/19 08:17:39, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/7030/8027#newcode42
Line 42: histogram_synchronizer_ = this;
On 2009/04/19 08:17:39, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/7030/8027#newcode47
Line 47: callback_task_ = NULL;
On 2009/04/19 08:17:39, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/7030/8027#newcode58
Line 58: // This method is called on the main UI thread from about:histograms.
On 2009/04/19 08:17:39, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/7030/8027#newcode60
Line 60: // Asert that the current thread is UI thread.
On 2009/04/19 08:17:39, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/7030/8027#newcode87
Line 87: DHISTOGRAM_COUNTS("Histogram.RendererHistogramsNotReceivedSynchrnous",
On 2009/04/19 08:17:39, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/7030/8027#newcode99
Line 99: // Asert that the current thread is UI thread.
On 2009/04/19 08:17:39, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/7030/8027#newcode106
Line 106: return;
On 2009/04/19 08:17:39, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/7030/8027#newcode143
Line 143: DCHECK(current_synchronizer->callback_task_ == NULL);
On 2009/04/19 08:17:39, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/7030/8027#newcode152
Line 152: // This method is called on the IO thread. Desrializes the histograms
and
On 2009/04/19 08:17:39, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/7030/8027#newcode157
Line 157: // Asert that the current thread is IO thread.
On 2009/04/19 08:17:39, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/7030/8027#newcode175
Line 175: // waiting process or call the callback funtion. This is called on IO
Thread.
On 2009/04/19 08:17:39, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/7030/8027#newcode177
Line 177: // Asert that the current thread is IO thread.
On 2009/04/19 08:17:39, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/7030/8027#newcode198
Line 198: // Asert that the current thread is IO thread.
On 2009/04/19 08:17:39, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/7030/8029
File chrome/common/histogram_synchronizer.h (right):

http://codereview.chromium.org/42496/diff/7030/8029#newcode24
Line 24: class HistogramSynchronizer;
On 2009/04/19 08:17:39, jar wrote:

Hi Jim,
  I was getting compilation errors. Would like to show this.

http://codereview.chromium.org/42496/diff/7030/8029#newcode38
Line 38: static HistogramSynchronizer* CurrentSynchronizer();
On 2009/04/19 08:17:39, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/7030/8029#newcode40
Line 40: void WaitForRendererHistograms();
On 2009/04/19 08:17:39, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/7030/8029#newcode42
Line 42: static void GetRendererHistogramsAsynchrnous(
On 2009/04/19 08:17:39, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/7030/8029#newcode51
Line 51: void CallCallbackTaskAndResetData();
On 2009/04/19 08:17:39, jar wrote:
Hi Jim,
  This method is not called from outside. Put all the common code that is called
from multiple places in this method.

http://codereview.chromium.org/42496/diff/7030/8029#newcode57
Line 57: // records that we have received the histograms from a renderer for the
given
On 2009/04/19 08:17:39, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/7030/8029#newcode58
Line 58: // sequence number. Returns true when we receive histograms from all
On 2009/04/19 08:17:39, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/7030/8029#newcode67
Line 67: // lock protects access to sequence number and histograms requested.
On 2009/04/19 08:17:39, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/7030/8029#newcode70
Line 70: ConditionVariable received_all_renderer_historgrams_;
On 2009/04/19 08:17:39, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/7030/8029#newcode72
Line 72: Task* callback_task_;
On 2009/04/19 08:17:39, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/7030/8029#newcode75
Line 75: // sequence number for collecting Renderer Histograms.
On 2009/04/19 08:17:39, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/7030/8029#newcode76
Line 76: int renderer_histogram_sequence_number_;
On 2009/04/19 08:17:39, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/7030/8029#newcode78
Line 78: int async_sequence_number_;
On 2009/04/19 08:17:39, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/7030/8029#newcode79
Line 79: int async_histograms_requested_;
On 2009/04/19 08:17:39, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/7030/8029#newcode81
Line 81: int synchronous_sequence_number_;
On 2009/04/19 08:17:39, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/7030/8029#newcode82
Line 82: int synchronous_histograms_requested_;
On 2009/04/19 08:17:39, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/7030/8029#newcode87
Line 87: static HistogramSynchronizer* histogram_synchronizer_;
On 2009/04/19 08:17:39, jar wrote:

Hi Jim,
  Moved the comment from CC file to here. 

Many many thanks for all the awesome comments

[raman, 2009-04-21 16:43:25]

Hi Jim,
  Added sequence number checking in the timeout function (forgot to make that
change earlier).

thanks,
raman

[raman, 2009-04-22 07:29:19]

Hi Jim,
  I have made all the changes you have suggested. Many thanks for all your help,
raman

http://codereview.chromium.org/42496/diff/7030/8027
File chrome/common/histogram_synchronizer.cc (right):

http://codereview.chromium.org/42496/diff/7030/8027#newcode126
Line 126: HistogramSynchronizer::ForceHistogramSynchronizationDoneCallback),
On 2009/04/19 08:17:39, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/7030/8029
File chrome/common/histogram_synchronizer.h (right):

http://codereview.chromium.org/42496/diff/7030/8029#newcode24
Line 24: class HistogramSynchronizer;
On 2009/04/21 15:05:44, raman wrote:

Deleted the forward declaration. It was unnecessary. thanks Jim.

http://codereview.chromium.org/42496/diff/7030/8029#newcode45
Line 45: static void SetCallbackTaskToCallAfterGettingRendererHistograms(
On 2009/04/19 08:17:39, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/7030/8029#newcode48
Line 48: static void ForceHistogramSynchronizationDoneCallback();
On 2009/04/19 08:17:39, jar wrote:

Done.

[jar (doing other things), 2009-04-23 04:36:21]

Very cool! :-)

Pretty much all my suggestions are now super tiny.

The only slightly larger item I'd be interested in adding is a histogram for the
asynch lookup.  Very few folks will actually do an about:histograms, but we will
see tons of async fetches for UMA.  So it would be good to understand how long
it really takes to do the fetches from the renderers. You already had the
synchronous lookup being timed... I'm just wondering what it will take on "real
user's" computers, etc.

I'm pretty sure I'll be able to land this pretty much as is (though I'd probably
clean up the nits).  Would you mind making the additions?

Thanks,

Jim

http://codereview.chromium.org/42496/diff/11020/12030
File chrome/common/histogram_synchronizer.cc (right):

http://codereview.chromium.org/42496/diff/11020/12030#newcode95
Line 95: // System teardown is happening.
This comment should be insider the following if block.

http://codereview.chromium.org/42496/diff/14030/13040
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/42496/diff/14030/13040#newcode912
Line 912: int wait_time = static_cast<int>(kInitialInterlogDuration * 1000 /
60);
Can you define a new constant:
kMaxHistogramGatheringWaitDuration
to be about 3 seconds (no reason to rush) and use it instead of the above
InterlogDuration

http://codereview.chromium.org/42496/diff/14030/13046
File chrome/common/histogram_synchronizer.cc (right):

http://codereview.chromium.org/42496/diff/14030/13046#newcode41
Line 41: delete callback_task_;
nit: It was pointed out to me that you're allowed to delete NULL, and hence the
test is not needed.

http://codereview.chromium.org/42496/diff/14030/13046#newcode67
Line 67: int renderer_histograms_not_received;
Nit: I'd suggest alternate variable name:
unresponsive_renderer_count

http://codereview.chromium.org/42496/diff/14030/13046#newcode79
Line 79: DHISTOGRAM_COUNTS("Histogram.RendererHistogramsNotReceivedSynchrnous",
nit: spelling typo: Synchrnous --> Synchronous

I'd actually suggest:
"Histogram.RendersNotRespondingSynchronous"

http://codereview.chromium.org/42496/diff/14030/13046#newcode210
Line 210:
DHISTOGRAM_COUNTS("Histogram.RendererHistogramsNotReceivedAsynchronous",
I'd suggest:
"Histogram.RendersNotRespondingAsynchronous"

http://codereview.chromium.org/42496/diff/14030/13046#newcode215
Line 215: callback_thread_->PostTask(FROM_HERE, callback_task_);
Perhaps we can add a histogram to see how long this took?  We'd need to save the
time when we were told to start the fetch in an instance like
async_callback_start_time_ and then histogram the delta.

http://codereview.chromium.org/42496/diff/14030/13046#newcode225
Line 225: ++next_avaible_sequence_number_;
nit: typo spelling: next_available_sequence_number_

http://codereview.chromium.org/42496/diff/14030/13048
File chrome/common/histogram_synchronizer.h (right):

http://codereview.chromium.org/42496/diff/14030/13048#newcode50
Line 50: // called on the UI thread from UMA.
Comment correction: This method is called on the IO thread from UMA via
PostMessage.

http://codereview.chromium.org/42496/diff/14030/13048#newcode62
Line 62: // Signal the waiting process or call the callback function. Returns
true when
nit: lower case S in Signal

http://codereview.chromium.org/42496/diff/14030/13048#newcode75
Line 75: // Method to get a new sequence number to be sent to Renderers from
broswer
nit: lower case R

http://codereview.chromium.org/42496/diff/14030/13048#newcode122
Line 122: // portion of main(), and hence it is not thread safe.  It initializes
I think you can delete the phrase:
", and hence it is not thread safe"

[raman, 2009-04-23 06:02:22]

Hi Jim,
  Made all the changes you have suggested. Would appreciate if you could take a
look.

thanks very much,
raman

http://codereview.chromium.org/42496/diff/11020/12030
File chrome/common/histogram_synchronizer.cc (right):

http://codereview.chromium.org/42496/diff/11020/12030#newcode95
Line 95: // System teardown is happening.
On 2009/04/23 04:36:22, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/14030/13040
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/42496/diff/14030/13040#newcode912
Line 912: int wait_time = static_cast<int>(kInitialInterlogDuration * 1000 /
60);
On 2009/04/23 04:36:22, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/14030/13046
File chrome/common/histogram_synchronizer.cc (right):

http://codereview.chromium.org/42496/diff/14030/13046#newcode41
Line 41: delete callback_task_;
On 2009/04/23 04:36:22, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/14030/13046#newcode67
Line 67: int renderer_histograms_not_received;
On 2009/04/23 04:36:22, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/14030/13046#newcode79
Line 79: DHISTOGRAM_COUNTS("Histogram.RendererHistogramsNotReceivedSynchrnous",
On 2009/04/23 04:36:22, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/14030/13046#newcode210
Line 210:
DHISTOGRAM_COUNTS("Histogram.RendererHistogramsNotReceivedAsynchronous",
On 2009/04/23 04:36:22, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/14030/13046#newcode215
Line 215: callback_thread_->PostTask(FROM_HERE, callback_task_);
On 2009/04/23 04:36:22, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/14030/13046#newcode225
Line 225: ++next_avaible_sequence_number_;
On 2009/04/23 04:36:22, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/14030/13048
File chrome/common/histogram_synchronizer.h (right):

http://codereview.chromium.org/42496/diff/14030/13048#newcode50
Line 50: // called on the UI thread from UMA.
On 2009/04/23 04:36:22, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/14030/13048#newcode62
Line 62: // Signal the waiting process or call the callback function. Returns
true when
On 2009/04/23 04:36:22, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/14030/13048#newcode75
Line 75: // Method to get a new sequence number to be sent to Renderers from
broswer
On 2009/04/23 04:36:22, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/14030/13048#newcode122
Line 122: // portion of main(), and hence it is not thread safe.  It initializes
On 2009/04/23 04:36:22, jar wrote:

Done.

[raman, 2009-04-23 13:09:26]

Hi Jim,
  I did a sync of the code as of this morning and uploaded the diffs again. 

  I will be going to India this evening for 1 week. I will be back on May 2nd
(Sat). I will start working on it after I come back. I am working from home
today. I will be able to fix the code until 1pm today.

thanks very much,
raman

[jar (doing other things), 2009-04-25 07:18:18]

Now that each method is clear about what thread it is on, I spotted a race bug
(I think).  The fix will be simple, but I'd like you to agree (see details
below).  I know you're away in India for a week, but if I just made the changes,
then I'd have to get a code review of the whole CL, so it seems better to get
agreement from you, so that my review can endorse your final CL.

Enjoy your trip.  I hope your family is well.

Jim

http://codereview.chromium.org/42496/diff/14092/14095
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/42496/diff/14092/14095#newcode898
Line 898: NewRunnableMethod(&MetricsService::CollectMemoryDetails),
Reading the code now, I think that CollectMemoryDetails() should be renamed
LogTransmissionTimerDone.
This really isn't an issue in this CL, but I'll put a TODO() comment.  We can
(some day) also re-order these methods, so that the fall sequentially in this
file in teh same order as they get run.  This is not critical for this CL, but
it is made clear by the nice names you have for your other callbacks.  We may
also be able to make these ...Done() methods private, which further enforces the
"don't call these directly" comments.

http://codereview.chromium.org/42496/diff/14092/14101
File chrome/common/histogram_synchronizer.cc (right):

http://codereview.chromium.org/42496/diff/14092/14101#newcode30
Line 30: async_callback_start_time_(TimeTicks::Now()),
You can use the null initializer, as this will be initialized when we set the
async callback.

http://codereview.chromium.org/42496/diff/14092/14101#newcode58
Line 58: SYNCHRONOUS_HISTOGRAMS, RenderProcessHost::size());
nit: indent 4 from previous line.

http://codereview.chromium.org/42496/diff/14092/14101#newcode113
Line 113: for (RenderProcessHost::iterator it = RenderProcessHost::begin();
...but now we're on the IO thread... so we may not be able to iterate on this
(without a race).
Perhaps we should run this method on the UI thread instead?

If so, it can be called directly from metrics_services, and we should be posting
to the IO thread, instead of current (or lines 103 and 119).

http://codereview.chromium.org/42496/diff/14092/14101#newcode130
Line 130: DCHECK(MessageLoop::current()->type() == MessageLoop::TYPE_IO);
I'd suggest we write a method called:
IsOnIoThread()
 for use ONLY in a DCHECK.  It should initialize in its first call, and then
consistently compare MessageLoop:;current() against a member called
io_message_loop_.
There are probably other threads of type IO, and we really want to be sure we're
consistently on the singular IO thread that guarantees we don't need to worry
about locks.

http://codereview.chromium.org/42496/diff/14092/14101#newcode154
Line 154: received_all_renderer_historgrams_.Signal();
Although not a big deal (as this is not performance critical) it is a good
practice to do a Signal() after you've released the lock.  If you think about
it, the other thread won't be able to start until you release the lock anyway. 
Some folks incorrectly assume that Signal() can only be called while holding the
lock, but this is not a restriction.  

If you restructure this if/else code a bit, you'll be able to exit the locked
scope and then do the signal.

http://codereview.chromium.org/42496/diff/14092/14103
File chrome/common/histogram_synchronizer.h (right):

http://codereview.chromium.org/42496/diff/14092/14103#newcode81
Line 81: // This lock_ protects access to sequence number and histograms
requested.
typo: change "and histogram requested" to "and synchronous_renderers_pending_"

[raman, 2009-05-03 19:27:29]

could you please look at this?

http://codereview.chromium.org/42496/diff/14092/14095
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/42496/diff/14092/14095#newcode898
Line 898: NewRunnableMethod(&MetricsService::CollectMemoryDetails),
On 2009/04/25 07:18:18, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/14092/14101
File chrome/common/histogram_synchronizer.cc (right):

http://codereview.chromium.org/42496/diff/14092/14101#newcode30
Line 30: async_callback_start_time_(TimeTicks::Now()),
On 2009/04/25 07:18:18, jar wrote:

Hi Jim,
  TimeTicks requires it to be initialized as Now(). When I initialized to NULL,
it gave an error.

http://codereview.chromium.org/42496/diff/14092/14101#newcode58
Line 58: SYNCHRONOUS_HISTOGRAMS, RenderProcessHost::size());
On 2009/04/25 07:18:18, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/14092/14101#newcode113
Line 113: for (RenderProcessHost::iterator it = RenderProcessHost::begin();
On 2009/04/25 07:18:18, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/14092/14101#newcode130
Line 130: DCHECK(MessageLoop::current()->type() == MessageLoop::TYPE_IO);
On 2009/04/25 07:18:18, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/14092/14101#newcode154
Line 154: received_all_renderer_historgrams_.Signal();
On 2009/04/25 07:18:18, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/14092/14103
File chrome/common/histogram_synchronizer.h (right):

http://codereview.chromium.org/42496/diff/14092/14103#newcode81
Line 81: // This lock_ protects access to sequence number and histograms
requested.
On 2009/04/25 07:18:18, jar wrote:

Done.

[jar (doing other things), 2009-05-04 18:05:00]

I looked at all the code (paying close attention to the changes)... and it sure
looks correct.  I have a few tiny nits below, but nothing substantial.

I'll try to patch a recent pull, and see what happens when I run it.

http://codereview.chromium.org/42496/diff/17001/18009
File chrome/common/histogram_synchronizer.cc (right):

http://codereview.chromium.org/42496/diff/17001/18009#newcode180
Line 180: } else {
This flow is much nicer!  Note that we don't need an "else" clause with the
braces and indent, as the "if true" clause does a return.

http://codereview.chromium.org/42496/diff/17001/18009#newcode182
Line 182: AutoLock auto_lock(lock_);
I'm not sure... but you should probably check the sequence number for a match
yet again after getting the lock, just in case it is changing asynchronously. 
If it doesn't match, we just return true (as you did just above when it didn't
match).

http://codereview.chromium.org/42496/diff/17001/18009#newcode270
Line 270: DCHECK(MessageLoop::current()->type() == MessageLoop::TYPE_IO);
If this is false, you should just return false so that we can see where this
test was failing.  Note that the caller always calls us in the context of a
DCHECK, which we can trigger with a return of false.

http://codereview.chromium.org/42496/diff/17001/18009#newcode274
Line 274: return true;
You'll be better off if you don't DCHECK at line 273, and then return the result
of the comparison contained in the DCHECK.  This way you'll get to fire at the
call site, which is the interesting question.

[raman, 2009-05-05 06:14:45]

Hi Jim,
  Made the changes you have suggested. 
thanks,
raman

http://codereview.chromium.org/42496/diff/17001/18009
File chrome/common/histogram_synchronizer.cc (right):

http://codereview.chromium.org/42496/diff/17001/18009#newcode180
Line 180: } else {
On 2009/05/04 18:05:00, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/17001/18009#newcode182
Line 182: AutoLock auto_lock(lock_);
On 2009/05/04 18:05:00, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/17001/18009#newcode270
Line 270: DCHECK(MessageLoop::current()->type() == MessageLoop::TYPE_IO);
On 2009/05/04 18:05:00, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/17001/18009#newcode274
Line 274: return true;
On 2009/05/04 18:05:00, jar wrote:

Done.

[jar (doing other things), 2009-05-07 22:02:55]

I figured out one way to work around the issue.. and then spoke with Darin and
confirmed it was indeed the sanctioned "pattern."  Sorry about the confusion I
might have caused by involving some of the more complex stuff, but it is better
to get this all straight.

I was also told that as an alternate to the approach listed below, we could look
at NewRunnableMethodTrait as a way to have a NewRunnableMethod without requiring
that the class be ref counted.

When you have time, I'd like to try to talk about the various patterns that
Darin and I discussed, including having a worker-object that is ref counted and
held by both these Tasks as well as the global singleton.  The approach didn't
quite work, since we had to respond to IPCs which didn't have a pointer :-(, but
I was assure that so long as we're running on the IO thread, we're allowed to
assume that the UI thread still has its live singletons :-).

http://codereview.chromium.org/42496/diff/19024/20028
File chrome/common/histogram_synchronizer.cc (right):

http://codereview.chromium.org/42496/diff/19024/20028#newcode114
Line 114:
current_synchronizer->histogram_synchronizer_factory_.NewRunnableMethod(
This was indeed the cause of teh run-time assert.  Apparently the limitation on
these factories is that they can't be used across threads <sigh>.  There are
some other interesting "patterns" that were suggested to me by Darin, but one of
the easiest it to use NewRunnableMethod(current_synchronizer, &SetCall.....).
instead of the factory.  ...but... to do such, you have to make this class
reference counted... and since there are releases and acquisitions on multiple
threads, you need to be RefCountedThreadSafe.  See the file base/ref_counted.h
for details and examples.

...and then... the singleton needs to be "held" in a ref counted ptr, rather
than just held in function scope... <big breadth> and it all works.

http://codereview.chromium.org/42496/diff/19024/20028#newcode137
Line 137:
current_synchronizer->histogram_synchronizer_factory_.NewRunnableMethod(
The same change applies here with regard to not using the factory since this is
cross-threaded.

[raman, 2009-05-08 07:01:31]

Hi Jim,
  I made the changes you have recommended. This code worked without hitting any
DCHECK. I hope I got the smart pointer stuff correct.

  Would appreciate if you could take a look at these changes.

thanks,
raman

http://codereview.chromium.org/42496/diff/19024/20028
File chrome/common/histogram_synchronizer.cc (right):

http://codereview.chromium.org/42496/diff/19024/20028#newcode114
Line 114:
current_synchronizer->histogram_synchronizer_factory_.NewRunnableMethod(
On 2009/05/07 22:02:55, jar wrote:

Done.

http://codereview.chromium.org/42496/diff/19024/20028#newcode137
Line 137:
current_synchronizer->histogram_synchronizer_factory_.NewRunnableMethod(
On 2009/05/07 22:02:55, jar wrote:

Done.

[jar (doing other things), 2009-05-08 23:27:25]

LGTM!  :-)

I'm pulling a an updated tree, and will merge onto that, and send you a CL.

Hopefully I'll be able to land next week.

THANKS!

Jim

[raman, 2009-05-12 05:29:01]

Hi JIm,
  Made the changes we have discussed this weekend (tried to add comments about
singleton in broswer_main.cc. Would appreciate if you could look at these
changes.

thanks much,
raman

[jar (doing other things), 2009-05-15 01:31:03]

LGTM

I'll pull this into my tree and run with a few days to be sure I don't see any
problems.

Thanks!!

Jim