Create a thread-safe observer list. Will be used

Create a thread-safe observer list.  Will be used
by SystemMonitor.

Right now the class requires that Observers be RefCounted<>.  This is because we invoke tasks via NewRunnableMethod for them.  However, because we manually track lifecycle via AddObserver/RemoveObserver, we could override the RunnableMethodTraits to not require RefCounted<>.   This would have the advantage that callers do not need to make all Observers be RefCounted, but makes it more critical that observers not forget to call RemoveObserver().

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=3787

[darin (slow to review), 2008-10-16 22:28:49]

http://codereview.chromium.org/7353/diff/26/29
File base/observer_list_threadsafe.h (right):

http://codereview.chromium.org/7353/diff/26/29#newcode190
Line 190: #define NOTIFY_OBSERVERS(ObserverType, observer_list, func, arg)      
      \
since you are making all notifications asynchronous, you can greatly simplify
this design.  just do a very simple list of observers that you directly mutate
from Add/RemoveObserver inside a lock.  when notifying all observers, grab the
lock and call PostTask for each observer.

another design would involve maintaining an ObserverList per thread.  when you
wish to notify observers, you could notify each ObserverList via PostTask, and
then each ObserverList could take care of notifying its respective observers. 
in this way, the multithreading complexity is isolated to a problem of
broadcasting notifications to N threads.

it may be very convenient to have a template class like Task that can be lazily
bound.

  template <class T, class Method, class Params>
  class UnboundMethod {
   public:
    UnboundMethod(Method m, Params p) : m_(m), p_(p) {
    }
    void Run(T* obj) {
      DispatchToMethod(obj, m_, p_);
    }
   private:
    Method m_;
    Params p_;
  };

You could then have a method on the ObserverListThreadSafe class that takes an
UnboundMethod object, that could be passed along to each ObserverList and then
you could Run that UnboundMethod on each observer in the list.

[Mike Belshe, 2008-10-17 22:18:15]

Thanks, Darin.

On 2008/10/16 22:28:49, darin wrote:
> http://codereview.chromium.org/7353/diff/26/29
> File base/observer_list_threadsafe.h (right):
> 
> http://codereview.chromium.org/7353/diff/26/29#newcode190
> Line 190: #define NOTIFY_OBSERVERS(ObserverType, observer_list, func, arg)    
 
>       \
> since you are making all notifications asynchronous, you can greatly simplify
> this design.  just do a very simple list of observers that you directly mutate
> from Add/RemoveObserver inside a lock.  when notifying all observers, grab the
> lock and call PostTask for each observer.

This is basically what the CL was doing.  You were right; there was some
extraneous code in the Compact() that was no longer necessary.


> 
> another design would involve maintaining an ObserverList per thread.  when you
> wish to notify observers, you could notify each ObserverList via PostTask, and
> then each ObserverList could take care of notifying its respective observers. 
> in this way, the multithreading complexity is isolated to a problem of
> broadcasting notifications to N threads.


I like this better because it gets more code reuse.  The templating is a bit
more crazy, but here is a new CL based on this model.

Look forward to your feedback!

> 
> it may be very convenient to have a template class like Task that can be
lazily
> bound.
> 
>   template <class T, class Method, class Params>
>   class UnboundMethod {
>    public:
>     UnboundMethod(Method m, Params p) : m_(m), p_(p) {
>     }
>     void Run(T* obj) {
>       DispatchToMethod(obj, m_, p_);
>     }
>    private:
>     Method m_;
>     Params p_;
>   };
> 
> You could then have a method on the ObserverListThreadSafe class that takes an
> UnboundMethod object, that could be passed along to each ObserverList and then
> you could Run that UnboundMethod on each observer in the list.

[darin (slow to review), 2008-10-17 22:27:45]

looks good... just some questions, and i think maybe a potential bug:

http://codereview.chromium.org/7353/diff/39/215
File base/observer_list_threadsafe.h (right):

http://codereview.chromium.org/7353/diff/39/215#newcode127
Line 127: list, new_method));
so just passing "method" by value doesn't work here?

would the template gods allow this:

  void NotifyWrapper(ObserverList<ObserverType>* list, const
UnboundMethod<ObserverType, Method, Params>& method) { ...

also, do we need to worry at all about the "list" parameter being destroyed
while the NewRunnableMethod is still in flight?

http://codereview.chromium.org/7353/diff/39/215#newcode153
Line 153: typedef std::map<MessageLoop*, ObserverList<ObserverType>*>
ObserversList;
nit: might be nice to name this ObserverListMap instead.  the name threw me a
bit.

[Mike Belshe, 2008-10-17 22:46:16]

On 2008/10/17 22:27:45, darin wrote:
> looks good... just some questions, and i think maybe a potential bug:
> 
> http://codereview.chromium.org/7353/diff/39/215
> File base/observer_list_threadsafe.h (right):
> 
> http://codereview.chromium.org/7353/diff/39/215#newcode127
> Line 127: list, new_method));
> so just passing "method" by value doesn't work here?
> 
> would the template gods allow this:
> 
>   void NotifyWrapper(ObserverList<ObserverType>* list, const
> UnboundMethod<ObserverType, Method, Params>& method) { ...

Yeah - actually - I think that does work.


> also, do we need to worry at all about the "list" parameter being destroyed
> while the NewRunnableMethod is still in flight?

The lists only get destroyed when the ThreadSafeObserverList is destroyed, and
the ThreadSafeObserverList is refcounted.  So, if there are items in flight,
then we posted NewRunnableMethods on it, which increased the refcount, and I
think this is okay.


> 
> http://codereview.chromium.org/7353/diff/39/215#newcode153
> Line 153: typedef std::map<MessageLoop*, ObserverList<ObserverType>*>
> ObserversList;
> nit: might be nice to name this ObserverListMap instead.  the name threw me a
> bit.

Changed.

[mendola, 2008-10-18 00:10:20]

General comments

http://codereview.chromium.org/7353/diff/45/221
File base/observer_list_threadsafe.h (right):

http://codereview.chromium.org/7353/diff/45/221#newcode6
Line 6: #define BASE_OBSERVER_THREADSAFE_LIST_H__
Class, guard and and filename have all different names :D

http://codereview.chromium.org/7353/diff/45/221#newcode57
Line 57: typename ObserversListMap::iterator it;
this iterator can be:  ObserverListMap::const_iterator;

http://codereview.chromium.org/7353/diff/45/221#newcode58
Line 58: for (it = observer_lists_.begin(); it != observer_lists_.end(); it++)
Use pre-increment;
Cache in a local variable observer_lists_.end()

http://codereview.chromium.org/7353/diff/45/221#newcode60
Line 60: observer_lists_.clear();
isn't this clear call superfluous ?

http://codereview.chromium.org/7353/diff/45/221#newcode67
Line 67: AutoLock lock(list_lock_);
swap line 67<->68 ? Better own the lock as less as possible.

http://codereview.chromium.org/7353/diff/45/221#newcode84
Line 84: AutoLock lock(list_lock_);
Same comment about line 67, 68

http://codereview.chromium.org/7353/diff/45/221#newcode86
Line 86: DCHECK(observer_lists_.find(loop) != observer_lists_.end()) <<
It's dangerous to keep this check only for the debug version

http://codereview.chromium.org/7353/diff/45/221#newcode110
Line 110: // TODO:  Add more wrappers for Notify() with mor arguments.
s/mor/more

http://codereview.chromium.org/7353/diff/45/221#newcode122
Line 122: &ThreadSafeObserverList<ObserverType>::template NotifyWrapper<Method,
Params>,
split the line

[Mike Belshe, 2008-10-20 04:03:10]

On 2008/10/18 00:10:20, mendola wrote:
> General comments

thanks!

> 
> http://codereview.chromium.org/7353/diff/45/221
> File base/observer_list_threadsafe.h (right):
> 
> http://codereview.chromium.org/7353/diff/45/221#newcode6
> Line 6: #define BASE_OBSERVER_THREADSAFE_LIST_H__
> Class, guard and and filename have all different names :D

ok

> 
> http://codereview.chromium.org/7353/diff/45/221#newcode57
> Line 57: typename ObserversListMap::iterator it;
> this iterator can be:  ObserverListMap::const_iterator;

ok

> 
> http://codereview.chromium.org/7353/diff/45/221#newcode58
> Line 58: for (it = observer_lists_.begin(); it != observer_lists_.end(); it++)
> Use pre-increment;
> Cache in a local variable observer_lists_.end()

I'm not buying the though-shalt-always-pre-increment nit.  I'd prefer we not nit
this unless there is specific reason to do so.   Do you have performance data to
indicate that this will have any impact whatsoever on this code?  As far as I
can tell, he cache adds more code without benefit.

> 
> http://codereview.chromium.org/7353/diff/45/221#newcode60
> Line 60: observer_lists_.clear();
> isn't this clear call superfluous ?

sort of, but its explicit.

> 
> http://codereview.chromium.org/7353/diff/45/221#newcode67
> Line 67: AutoLock lock(list_lock_);
> swap line 67<->68 ? Better own the lock as less as possible.

ok
> 
> http://codereview.chromium.org/7353/diff/45/221#newcode84
> Line 84: AutoLock lock(list_lock_);
> Same comment about line 67, 68

ok
> 
> http://codereview.chromium.org/7353/diff/45/221#newcode86
> Line 86: DCHECK(observer_lists_.find(loop) != observer_lists_.end()) <<
> It's dangerous to keep this check only for the debug version

Yes, lots of things are dangerous...  Observers can also verify that they are
being used on the proper thread.  But I would guess most use dchecks as well.

> 
> http://codereview.chromium.org/7353/diff/45/221#newcode110
> Line 110: // TODO:  Add more wrappers for Notify() with mor arguments.
> s/mor/more

ok

> 
> http://codereview.chromium.org/7353/diff/45/221#newcode122
> Line 122: &ThreadSafeObserverList<ObserverType>::template
NotifyWrapper<Method,
> Params>,
> split the line

ok

[mendola, 2008-10-20 10:33:07]

On 2008/10/20 04:03:10, mbelshe wrote:
> On 2008/10/18 00:10:20, mendola wrote:
> > General comments
> 
> thanks!

you are welcome :D


> > http://codereview.chromium.org/7353/diff/45/221#newcode58
> > Line 58: for (it = observer_lists_.begin(); it != observer_lists_.end();
it++)
> > Use pre-increment;
> > Cache in a local variable observer_lists_.end()
> 
> I'm not buying the though-shalt-always-pre-increment nit.  I'd prefer we not
nit
> this unless there is specific reason to do so.   Do you have performance data
to
> indicate that this will have any impact whatsoever on this code?  As far as I
> can tell, he cache adds more code without benefit.

The iterator for a map is not a primitive type, and this what I
get from headers on my compiler for the two implementations:

      _Self&
      operator++()
      {
        _M_node = _Rb_tree_increment(_M_node);
        return *this;
      }

      _Self
      operator++(int)
      {
        _Self __tmp = *this;
        _M_node = _Rb_tree_increment(_M_node);
        return __tmp;
      }

I would call the ++it, I don't rely on compilers about optimizing
that code. 

Gaetano

[mendola, 2008-10-20 10:35:27]

Typo on the guard name check

http://codereview.chromium.org/7353/diff/49/232
File base/observer_list_threadsafe.h (right):

http://codereview.chromium.org/7353/diff/49/232#newcode5
Line 5: #ifndef BASE_OBSERVER_LIST_THREADSAFE
It should be BASE_OBSERVER_LIST_THREADSAFE_H__

[Mike Belshe, 2008-10-20 16:14:20]

Regarding the ++it/it++; unless you're in a perf critical loop, it doesn't
matter - even if the compiler doesn't optimize it.  Nitting this stuff
generically is not worth doing unless its going to have an effect.  I made the
change, but I don't think we should nit this - its an example of
over-optimization :-)

On 2008/10/20 10:33:07, mendola wrote:
> On 2008/10/20 04:03:10, mbelshe wrote:
> > On 2008/10/18 00:10:20, mendola wrote:
> > > General comments
> > 
> > thanks!
> 
> you are welcome :D
> 
> 
> > > http://codereview.chromium.org/7353/diff/45/221#newcode58
> > > Line 58: for (it = observer_lists_.begin(); it != observer_lists_.end();
> it++)
> > > Use pre-increment;
> > > Cache in a local variable observer_lists_.end()
> > 
> > I'm not buying the though-shalt-always-pre-increment nit.  I'd prefer we not
> nit
> > this unless there is specific reason to do so.   Do you have performance
data
> to
> > indicate that this will have any impact whatsoever on this code?  As far as
I
> > can tell, he cache adds more code without benefit.
> 
> The iterator for a map is not a primitive type, and this what I
> get from headers on my compiler for the two implementations:
> 
>       _Self&
>       operator++()
>       {
>         _M_node = _Rb_tree_increment(_M_node);
>         return *this;
>       }
> 
>       _Self
>       operator++(int)
>       {
>         _Self __tmp = *this;
>         _M_node = _Rb_tree_increment(_M_node);
>         return __tmp;
>       }
> 
> I would call the ++it, I don't rely on compilers about optimizing
> that code. 
> 
> Gaetano

[darin (slow to review), 2008-10-20 22:25:35]

Just some trivial nits and a question about garbage collection.

http://codereview.chromium.org/7353/diff/236/62
File base/build/base.vcproj (right):

http://codereview.chromium.org/7353/diff/236/62#newcode497
Line 497: RelativePath="..\observer_list_threadsafe.h"
nit: this file name tells me that the class name is ObserverListThreadsafe, but
that doesn't match the class name you used.

http://codereview.chromium.org/7353/diff/236/60
File base/observer_list_threadsafe.h (right):

http://codereview.chromium.org/7353/diff/236/60#newcode5
Line 5: #ifndef BASE_OBSERVER_LIST_THREADSAFE_H__
nit: new code should follow the new style guide, which says to only use one
trailing "_"

http://codereview.chromium.org/7353/diff/236/60#newcode44
Line 44: //   The ThreadSafeObserver maintains an ObserverList for each thread
nit: ThreadSafeObserverList?

http://codereview.chromium.org/7353/diff/236/60#newcode90
Line 90: list->RemoveObserver(obs);
When do we garbage collect the list?  Shouldn't we remove it from the map if it
becomes empty as a result of this removal?

I know such a change would have implications on any pending call to
NotifyWrapper, but that could be dealt with by having NotifyWrapper lookup the
ObserverList given MessageLoop::current().

http://codereview.chromium.org/7353/diff/236/60#newcode97
Line 97: // been Notified.  The notification may still be pending delivery.
This comment should include a strong WARNING about the fact that you should
avoid passing pointers or references to stack allocated data since that data
needs to stay alive until all observers have been notified.

I predict that we will be fixing crashers related to this issue in the future :(

http://codereview.chromium.org/7353/diff/236/60#newcode110
Line 110: // TODO:  Add more wrappers for Notify() with more arguments.
nit: TODO(mbelshe)

http://codereview.chromium.org/7353/diff/236/60#newcode148
Line 148: typedef std::map<MessageLoop*, ObserverList<ObserverType>*>
ObserversListMap;
nit: ObserverListMap (note the absent "s") since it is a map of ObserverList
objects.

http://codereview.chromium.org/7353/diff/236/58
File base/observer_list_unittest.cc (right):

http://codereview.chromium.org/7353/diff/236/58#newcode211
Line 211: void ThreadSafeObserverHarness(int num_threads, bool
cross_thread_notifies) {
nit: this method should be marked static or moved into an anonymous namespace.

[Mike Belshe, 2008-10-21 21:53:32]

On 2008/10/20 22:25:35, darin wrote:
> Just some trivial nits and a question about garbage collection.
> 
> http://codereview.chromium.org/7353/diff/236/62
> File base/build/base.vcproj (right):
> 
> http://codereview.chromium.org/7353/diff/236/62#newcode497
> Line 497: RelativePath="..\observer_list_threadsafe.h"
> nit: this file name tells me that the class name is ObserverListThreadsafe,
but
> that doesn't match the class name you used.

OK - changed to ObserverListThreadSafe.

> 
> http://codereview.chromium.org/7353/diff/236/60
> File base/observer_list_threadsafe.h (right):
> 
> http://codereview.chromium.org/7353/diff/236/60#newcode5
> Line 5: #ifndef BASE_OBSERVER_LIST_THREADSAFE_H__
> nit: new code should follow the new style guide, which says to only use one
> trailing "_"

ok

> 
> http://codereview.chromium.org/7353/diff/236/60#newcode44
> Line 44: //   The ThreadSafeObserver maintains an ObserverList for each thread
> nit: ThreadSafeObserverList?
> 
> http://codereview.chromium.org/7353/diff/236/60#newcode90
> Line 90: list->RemoveObserver(obs);
> When do we garbage collect the list?  Shouldn't we remove it from the map if
it
> becomes empty as a result of this removal?

I didn't think this was a likely interesting case.  But I suppose if we someday
add transient threads it could be.

> 
> I know such a change would have implications on any pending call to
> NotifyWrapper, but that could be dealt with by having NotifyWrapper lookup the
> ObserverList given MessageLoop::current().

ok.

> 
> http://codereview.chromium.org/7353/diff/236/60#newcode97
> Line 97: // been Notified.  The notification may still be pending delivery.
> This comment should include a strong WARNING about the fact that you should
> avoid passing pointers or references to stack allocated data since that data
> needs to stay alive until all observers have been notified.
> 
> I predict that we will be fixing crashers related to this issue in the future
:(

I think we could add some debug assert logic for checking if a pointer is stack
based.  It may not be perfect, but it wouldn't have false positives.

> 
> http://codereview.chromium.org/7353/diff/236/60#newcode110
> Line 110: // TODO:  Add more wrappers for Notify() with more arguments.
> nit: TODO(mbelshe)

I don't really want my name on the TODO :-)  Basically, this is a note that
functions with additional arguments could be added here.  I'll make your change
so it is copacetic, but I was looking for something else....

> 
> http://codereview.chromium.org/7353/diff/236/60#newcode148
> Line 148: typedef std::map<MessageLoop*, ObserverList<ObserverType>*>
> ObserversListMap;
> nit: ObserverListMap (note the absent "s") since it is a map of ObserverList
> objects.
> 
> http://codereview.chromium.org/7353/diff/236/58
> File base/observer_list_unittest.cc (right):
> 
> http://codereview.chromium.org/7353/diff/236/58#newcode211
> Line 211: void ThreadSafeObserverHarness(int num_threads, bool
> cross_thread_notifies) {
> nit: this method should be marked static or moved into an anonymous namespace.

ok

[Mike Belshe, 2008-10-22 22:04:22]

I went ahead and made the cleanups work.  Took a couple of iterations, but I
think it is all good now.  The existing unit test does a good job of exercising
the code path where we add/remove all Observers for a given MessageLoop, thus
causing the ObserverListThreadSafe to delete the entire list, and potentially
create a new one.


On 2008/10/21 21:53:32, mbelshe wrote:
> On 2008/10/20 22:25:35, darin wrote:
> > Just some trivial nits and a question about garbage collection.
> > 
> > http://codereview.chromium.org/7353/diff/236/62
> > File base/build/base.vcproj (right):
> > 
> > http://codereview.chromium.org/7353/diff/236/62#newcode497
> > Line 497: RelativePath="..\observer_list_threadsafe.h"
> > nit: this file name tells me that the class name is ObserverListThreadsafe,
> but
> > that doesn't match the class name you used.
> 
> OK - changed to ObserverListThreadSafe.
> 
> > 
> > http://codereview.chromium.org/7353/diff/236/60
> > File base/observer_list_threadsafe.h (right):
> > 
> > http://codereview.chromium.org/7353/diff/236/60#newcode5
> > Line 5: #ifndef BASE_OBSERVER_LIST_THREADSAFE_H__
> > nit: new code should follow the new style guide, which says to only use one
> > trailing "_"
> 
> ok
> 
> > 
> > http://codereview.chromium.org/7353/diff/236/60#newcode44
> > Line 44: //   The ThreadSafeObserver maintains an ObserverList for each
thread
> > nit: ThreadSafeObserverList?
> > 
> > http://codereview.chromium.org/7353/diff/236/60#newcode90
> > Line 90: list->RemoveObserver(obs);
> > When do we garbage collect the list?  Shouldn't we remove it from the map if
> it
> > becomes empty as a result of this removal?
> 
> I didn't think this was a likely interesting case.  But I suppose if we
someday
> add transient threads it could be.
> 
> > 
> > I know such a change would have implications on any pending call to
> > NotifyWrapper, but that could be dealt with by having NotifyWrapper lookup
the
> > ObserverList given MessageLoop::current().
> 
> ok.
> 
> > 
> > http://codereview.chromium.org/7353/diff/236/60#newcode97
> > Line 97: // been Notified.  The notification may still be pending delivery.
> > This comment should include a strong WARNING about the fact that you should
> > avoid passing pointers or references to stack allocated data since that data
> > needs to stay alive until all observers have been notified.
> > 
> > I predict that we will be fixing crashers related to this issue in the
future
> :(
> 
> I think we could add some debug assert logic for checking if a pointer is
stack
> based.  It may not be perfect, but it wouldn't have false positives.
> 
> > 
> > http://codereview.chromium.org/7353/diff/236/60#newcode110
> > Line 110: // TODO:  Add more wrappers for Notify() with more arguments.
> > nit: TODO(mbelshe)
> 
> I don't really want my name on the TODO :-)  Basically, this is a note that
> functions with additional arguments could be added here.  I'll make your
change
> so it is copacetic, but I was looking for something else....
> 
> > 
> > http://codereview.chromium.org/7353/diff/236/60#newcode148
> > Line 148: typedef std::map<MessageLoop*, ObserverList<ObserverType>*>
> > ObserversListMap;
> > nit: ObserverListMap (note the absent "s") since it is a map of ObserverList
> > objects.
> > 
> > http://codereview.chromium.org/7353/diff/236/58
> > File base/observer_list_unittest.cc (right):
> > 
> > http://codereview.chromium.org/7353/diff/236/58#newcode211
> > Line 211: void ThreadSafeObserverHarness(int num_threads, bool
> > cross_thread_notifies) {
> > nit: this method should be marked static or moved into an anonymous
namespace.
> 
> ok

[darin (slow to review), 2008-10-22 22:09:03]

LGTM