Proof of concept for tr1-based Task replacement.

base/closure.h

Index: base/closure.h
diff --git a/base/closure.h b/base/closure.h
new file mode 100644
index 0000000000000000000000000000000000000000..ca891c7879bd9dfa65a630ba4dee9cbf545e6cd7
--- /dev/null
+++ b/base/closure.h
@@ -0,0 +1,338 @@
+// Copyright (c) 2009 The Chromium Authors. All rights reserved.

[brettw, 2011/01/04 19:12:17]

Copyright is 2 years out of date.

[awong, 2011/01/04 20:57:13]

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef BASE_CLOSURE_H_
+#define BASE_CLOSURE_H_
+#pragma once
+
+#include <tr1/functional>
+
+#include "base/ref_counted.h"
+#include "base/scoped_ptr.h"
+#include "base/tracked.h"
+
+namespace base {
+
+extern void DoNothing();  // Defined in base/task.h.
+
+// We are trying to mimic value-object semantics of tr1::function in the
+// Closure API.  Thus, we need to detach the tracked object, and some other
+// state as a shared state.
+//
+// As is, there are some threadsafety issues, but the general idea should be
+// correct.
+class ClosureState : public RefCountedThreadSafe<ClosureState> {
+ public:
+  // TODO(ajwong): Put these behind an accessor.
+  ::std::tr1::function<void()> release_thunk_;
+
+  explicit ClosureState(tracked_objects::Tracked* tracked)

[brettw, 2011/01/04 19:12:17]

I think we'd want the implementation of all of the non-template functions to be
in a .cc file to avoid bloat.

[awong, 2011/01/04 20:57:13]

Right...I'd like to avoid breaking out a cc file for right now since this is
mainly a proof-of-concept CL for testing out the API and showing how the basic
components might look.

Added a TODO at the top of the file.

+      : tracked_(tracked) {
+  }
+
+  ~ClosureState() {
+    release_thunk_();

[willchan no longer on Chromium, 2011/01/04 22:27:23]

Why?  We don't do this for Tasks.

+  }
+
+  tracked_objects::Tracked* tracked() const {
+    return tracked_.get();
+  }
+
+ private:
+  scoped_ptr<tracked_objects::Tracked> tracked_;

[willchan no longer on Chromium, 2011/01/04 22:27:23]

I'm not sure, but this might be a design flaw.  ClosureState is shared. 
tracked_objects::Tracked is designed to be used for a single task run on an
executor (MessageLoop or WorkerPool or what not).

So, if you intend a Closure to be allowed to be "permanent", then it needs to be
postable multiple times.  This means each time it's posted, it needs a new
tracked_objects::Tracked.

Really, that we make Task inherit from tracked_objects::Tracked seems like a
design flaw.  I think that tracked_objects::Tracked should be a sibling object
of the Task, contained within PendingTask.  Then this would work properly.

+};

[brettw, 2011/01/04 19:12:17]

DISALLOW_COPY_AND_ASSIGN?

[awong, 2011/01/04 20:57:13]

Done.

+
+
+// These are needed to implement a wrapper that disables refcounting on
+// objects used with a Closure.
+//
+// TODO(ajwong): If we implement this similar to std::tr1::reference_wrapper we
+// can probably simplify the ObjectTraits below and remove one specialization.
+template <typename O>
+class UnretainedWrapper {
+ public:
+  explicit UnretainedWrapper(O* o) : obj_(o) {}
+  O* get() { return obj_; }
+
+ private:
+  O* obj_;
+};
+
+template <typename O>
+UnretainedWrapper<O> Unretained(O* o) {
+  return UnretainedWrapper<O>(o);
+}
+
+// Note, Closure does not support binding a member function to a reference
+// object.  That is, this will not compile:

[brettw, 2011/01/04 19:12:17]

I find the existence of this "wrong" code confusing since I often just scan for
code samples and copy them. I'd just delete the "wrong" example and say
  // object. You have to pass in a pointer to it:
  //
     <right code>

It would be nice if there was also an example of using a Closure with
parameters.

[awong, 2011/01/04 20:57:13]

Removed the incorrect code, and added a comment.

Once an API is settled on, I'll beef up the documentation a lot.  There's a
moderate amount of template magic going on as well that should be commented.

+//
+//   Foo f;
+//   Closure c = Closure(&Foo::func, f);
+//
+// You have to pass in a pointer to it.
+//
+//   Foo f;
+//   Closure c = Closure(&Foo::func, &f);
+//
+// We can make the syntax work out, but it's extra complex and given that we
+// expect Closure's to refcount the member functions they abstract unless
+// explicitly told not to, allowing pass by reference doesn't make much sense.
+class Closure {
+ public:
+
+// --- Traits object defintions --

[brettw, 2011/01/04 19:12:17]

I'd make sure everything inside the class is indented 2 spaces. I've not seen us
do this non-indented comment style anywhere else.

[awong, 2011/01/04 20:57:13]

fixed.

+//  This handle changing the syntax, and refcounting for the various types of
+//  parameters that can be passed in.
+
+  template <typename O, bool is_member_function_pointer>
+  struct ObjectTraits;
+
+  // Handle regular pointers.
+  template <typename O>
+  struct ObjectTraits<O*, true> {
+    static O* Unwrap(O* o) {
+      return o;
+    }
+
+    static ::std::tr1::function<void()> RefThunk(O* o) {

[brettw, 2011/01/04 19:12:17]

I've never seen us write "::std" (everywhere we just do "std::"). It seems just
as clear so I'd use that instead.

[awong, 2011/01/04 20:57:13]

Done.

+      return ::std::tr1::bind(&O::AddRef, o);
+    }
+    static ::std::tr1::function<void()> ReleaseThunk(O* o) {
+      return ::std::tr1::bind(&O::Release, o);
+    }
+  };
+
+  // If it's not a member function, there is no refcounting, and pass through
+  // the object.
+  template <typename O>
+  struct ObjectTraits<O, false> {
+    static O Unwrap(O o) {
+      return o;
+    }
+
+    static ::std::tr1::function<void()> RefThunk(O o) {
+      return ::std::tr1::bind(&DoNothing);
+    }
+    static ::std::tr1::function<void()> ReleaseThunk(O o) {
+      return ::std::tr1::bind(&DoNothing);
+    }
+  };
+
+  // Object wrapped in Unretained() should not refcount anything, and should
+  // unwrap to a pointer to their type.
+  template <typename O>
+  struct ObjectTraits<UnretainedWrapper<O>, true> {
+    static O* Unwrap(UnretainedWrapper<O> o) {
+      return o.get();
+    }
+
+    static ::std::tr1::function<void()> RefThunk(UnretainedWrapper<O> o) {
+      return ::std::tr1::bind(&DoNothing);
+    }
+
+    static ::std::tr1::function<void()> ReleaseThunk(UnretainedWrapper<O> o) {
+      return ::std::tr1::bind(&DoNothing);
+    }
+  };
+
+
+// ---- Constructors ----
+  template <typename Sig>
+  Closure(Sig f)
+    : state_(new ClosureState(new tracked_objects::Tracked())) {

[brettw, 2011/01/04 19:12:17]

All these wrapped initializers should be indented 2 more spaces (: indented 4
spaces).

[awong, 2011/01/04 20:57:13]

Done.

+    func_ = ::std::tr1::bind(f);
+    state_->release_thunk_ = ::std::tr1::bind(&DoNothing);
+  }
+
+  template <typename Sig, typename A1>
+  Closure(Sig f, A1 a1)
+    : state_(new ClosureState(new tracked_objects::Tracked())) {
+    using std::tr1::is_member_function_pointer;
+    func_ = ::std::tr1::bind(
+        f,
+        ObjectTraits<A1, is_member_function_pointer<Sig>::value >::Unwrap(a1));
+
+    // Handle the refcounting.
+    ObjectTraits<A1, is_member_function_pointer<Sig>::value >::RefThunk(a1)();
+    state_->release_thunk_ =
+        ObjectTraits<A1, is_member_function_pointer<Sig>::value >::
+            ReleaseThunk(a1);
+  }
+
+  template <typename Sig, typename A1,  typename A2>
+  Closure(Sig f, A1 a1, A2 a2)
+    : state_(new ClosureState(new tracked_objects::Tracked())) {
+    using std::tr1::is_member_function_pointer;
+    func_ = ::std::tr1::bind(
+        f,
+        ObjectTraits<A1, is_member_function_pointer<Sig>::value >::Unwrap(a1),
+        a2);
+
+    // Handle the refcounting.
+    ObjectTraits<A1, is_member_function_pointer<Sig>::value >::RefThunk(a1)();
+    state_->release_thunk_ =
+        ObjectTraits<A1, is_member_function_pointer<Sig>::value >::
+            ReleaseThunk(a1);
+  }
+
+  template <typename Sig, typename A1,  typename A2,  typename A3>
+  Closure(Sig f, A1 a1, A2 a2, A3 a3)
+    : state_(new ClosureState(new tracked_objects::Tracked())) {
+    using std::tr1::is_member_function_pointer;
+    func_ = ::std::tr1::bind(
+        f,
+        ObjectTraits<A1, is_member_function_pointer<Sig>::value >::Unwrap(a1),
+        a2,
+        a3);
+
+    // Handle the refcounting.
+    ObjectTraits<A1, is_member_function_pointer<Sig>::value >::RefThunk(a1)();
+    state_->release_thunk_ =
+        ObjectTraits<A1, is_member_function_pointer<Sig>::value >::
+            ReleaseThunk(a1);
+  }
+
+  template <typename Sig, typename A1,  typename A2,  typename A3,
+           typename A4>
+  Closure(Sig f, A1 a1, A2 a2, A3 a3, A4 a4)
+    : state_(new ClosureState(new tracked_objects::Tracked())) {

[brettw, 2011/01/04 19:12:17]

bind makes an object on the heap underneath, right? And here you're making two
more heap objects. For something as simple as Closure(&Foo::func, &f) I wouldn't
necessarily expect 3 heap allocations, especially since I'm allocating the
Closure itself on the stack.

I don't think NewRunnableMethod allocates *any* objects on the heap (am I
missing something?), so making it now have 3 seems like a significant
regression.

[brettw, 2011/01/04 19:14:58]

Actually NewRunnableMethod of course make a "new" runnable method, which I think
is equivalent to the new inside of bind? So it's 3 versus 1?

[awong, 2011/01/04 20:57:13]

Right...really, the tr1::function<> reference should be moved into the closure
state, and ClosureState should just contain a Tracked object.  That should move
this from 3 allocations down to 2.  Also, passing/copying a closure object would
become just one scoped_refptr copy.

I think 2 allocations is as low as can be gotten with any callback system that
wraps another unless you start playing with placement-new.  To get back to 1 one
allocation, we'd need to drop the tr1::bind dependency.  This might be a good
enough motivation to avoid tr1::bind.

[willchan no longer on Chromium, 2011/01/04 22:27:23]

I don't think tr1::bind exposes the allocator as a template parameter, so I
don't know of any way to use placement new to get around this limitation.

Stepping back a second, I want to revisit the whole wrapping thing.  As I
understand it, the reason to wrap it is to simplify the API to make it easier to
understand by hiding the details.  I think, that's possible without any
overhead, if eliminate ClosureState.  I've proposed removing
tracked_objects::Tracked from ClosureState, which I believe eliminates any
reason to have ClosureState.  Closure should basically be tr1::function, with a
more chromium-styled API to make it blend more easily into our codebase.

+    using std::tr1::is_member_function_pointer;
+    func_ = ::std::tr1::bind(
+        f,
+        ObjectTraits<A1, is_member_function_pointer<Sig>::value >::Unwrap(a1),
+        a2,
+        a3,
+        a4);
+
+    // Handle the refcounting.
+    ObjectTraits<A1, is_member_function_pointer<Sig>::value >::RefThunk(a1)();
+    state_->release_thunk_ =
+        ObjectTraits<A1, is_member_function_pointer<Sig>::value >::
+            ReleaseThunk(a1);
+  }
+
+  template <typename Sig, typename A1,  typename A2,  typename A3,
+           typename A4,  typename A5>
+  Closure(Sig f, A1 a1, A2 a2, A3 a3, A4 a4, A5 a5)

[brettw, 2011/01/04 19:12:17]

In NewRunnableMethod we pass the arguments as const refs. It seems like that
will prevent a copy of objects passed around (say like a GURL).

[awong, 2011/01/04 20:57:13]

Eek...good point.  I was copying the tr1::bind API.  We can change this to take
const AN& as the argument type.

However, this points to another mark against wrapping tr1::bind's
implementation.

I just did a test run with gcc's tr1::bind implementation and found it copies
the argument 4 times on a bind call.  For NRM, it copies the argument 2 times.

[willchan no longer on Chromium, 2011/01/04 21:09:22]

Is this correct?  NewRunnableMethod() will create a Params, which is a struct
Tuple type, which has member variables which make a copy of the parameter.  We
definitely make a copy.  Otherwise, const std::string& as a parameter to
NewRunnableMethod() would cause a bunch of bugs.

+    : state_(new ClosureState(new tracked_objects::Tracked())) {
+    using std::tr1::is_member_function_pointer;
+    func_ = ::std::tr1::bind(
+        f,
+        ObjectTraits<A1, is_member_function_pointer<Sig>::value >::Unwrap(a1),
+        a2,
+        a3,
+        a4,
+        a5);
+
+    // Handle the refcounting.
+    ObjectTraits<A1, is_member_function_pointer<Sig>::value >::RefThunk(a1)();
+    state_->release_thunk_ =
+        ObjectTraits<A1, is_member_function_pointer<Sig>::value >::
+            ReleaseThunk(a1);
+  }
+
+  template <typename Sig, typename A1,  typename A2,  typename A3,
+           typename A4,  typename A5, typename A6>
+  Closure(Sig f, A1 a1, A2 a2, A3 a3, A4 a4, A5 a5, A6 a6)
+    : state_(new ClosureState(new tracked_objects::Tracked())) {
+    using std::tr1::is_member_function_pointer;
+    func_ = ::std::tr1::bind(
+        f,
+        ObjectTraits<A1, is_member_function_pointer<Sig>::value >::Unwrap(a1),
+        a2,
+        a3,
+        a4,
+        a5,
+        a6);
+
+    // Handle the refcounting.
+    ObjectTraits<A1, is_member_function_pointer<Sig>::value >::RefThunk(a1)();
+    state_->release_thunk_ =
+        ObjectTraits<A1, is_member_function_pointer<Sig>::value >::
+            ReleaseThunk(a1);
+  }
+
+  // This is to make Closure mimic tr1::function<void()>'s interface.  It could
+  // also just be void Run(), but mimicing the interface allows us to
+  // substitute Closure in whereever tr1::function<void()> could be used in the
+  // tr1 libraries.
+  void operator()() { func_(); }
+
+  tracked_objects::Tracked* tracked() {
+    return state_->tracked();
+  }
+
+  void Cancel() {
+    func_ = ::std::tr1::bind(&DoNothing);
+  }
+
+ private:
+  // Don't let us convert from tr1 quickly.
+  Closure(::std::tr1::function<void(void)> f)
+    : state_(new ClosureState(new tracked_objects::Tracked())) {
+    func_ = ::std::tr1::bind(f);
+  }
+
+  // This is the tr1 function pointer.
+  ::std::tr1::function<void()> func_;
+
+  scoped_refptr<ClosureState> state_;
+};
+
+// Wraps a Closure or a tr1::function<void()) to automatically cancel a task
+// when the ClosureCanceller is deleted.  This allows a caller to "nop" all
+// outstanding callbacks registered with the ClosureCanceller.
+class ClosureCanceller {
+ public:
+  ClosureCanceller() : cancel_state_(new CancelState()) {}

[brettw, 2011/01/04 19:12:17]

Ditto for non-inlining these.

[awong, 2011/01/04 20:57:13]

deferring addressing this for now.

+
+  ~ClosureCanceller() {
+    cancel_state_->is_canceled = true;
+  }
+
+  template <typename T>
+  Closure Wrap(T c) {
+    using ::std::tr1::bind;
+    return base::Closure(bind(&ClosureCanceller::Run<T>, cancel_state_, c));
+  }
+
+  bool empty() const {

[brettw, 2011/01/04 19:12:17]

Since this calls an uppercase (non-inlinable-for-free) function, I'd expect it
to be called IsEmpty() and non-inline.

[awong, 2011/01/04 20:57:13]

Was replicating the ScopedRunnableMethodFactory API and implementation.  FYI,
SRMF violates this itself...

I added a TODO.

+    // The ClosureCanceller has the only reference, no tasks are outstanding.
+    return cancel_state_->HasOneRef();
+  }
+
+  void RevokeAll() {
+    // Cancel all outstanding, then create a new cancel state so this object may
+    // be reused.
+    cancel_state_->is_canceled = true;
+    cancel_state_ = new CancelState();
+  }
+
+ private:
+  // The ScopedRunnableMethodFactory uses a WeakPtr.  This is because it is
+  // actually reimplementing the storage for the task object, so a pointer is
+  // necessary and thus WeakPtr can be overloaded to serve as a flag.
+  //
+  // In this design, it seems overkill to use WeakPtr instead of a simple flag
+  // class (which WeakPtr eventually devolves into anyways).
+  class CancelState : public RefCounted<CancelState> {

[brettw, 2011/01/04 19:12:17]

The definition of this can probably be moved to the .cc file and just forward
declare this inner class.

[awong, 2011/01/04 20:57:13]

deferred via TODO at the top of the file.

+   public:
+    CancelState() : is_canceled(false) {}
+
+    bool is_canceled;
+  };
+
+  template <typename T>
+  static void Run(scoped_refptr<CancelState> cancel_state, T c) {
+    if (!cancel_state->is_canceled) {
+      c();
+    }
+  }
+
+  scoped_refptr<CancelState> cancel_state_;
+};

[brettw, 2011/01/04 19:12:17]

DISALLOW_COPY_AND_ASSIGN?

[awong, 2011/01/04 20:57:13]

Done.

+
+}  // namespace base
+
+#endif // BASE_CLOSURE_H_