NFD » ndn-cxx

The name of this task should be "simplify", not optimize. The suggested implementation is equivalent, just cleaner.

Do you intend to provide an overload operator+=(Handle) in addition to the existing operator+=(std::function<>)) or completely remove the latter?

Alternative to the design is to use more conventional syntax to subscribe, which would return some kind of ID for the subscription. This ID could then be used to unsubscribe:

OpaqueId
subscribe(std::function<>);

void
unsubscribe(OpaqueId);

I intend to provide overloaded operator+=.

I'm trying to mimic C# syntax, so I'll use operator overloading.

Junxiao Shi wrote:

I intend to provide overloaded operator+=.

I'm trying to mimic C# syntax, so I'll use operator overloading.

Can you give example of this C# syntax? I'm not entirely convinced that for the sake of interface purity one will have to write more code than is necessary (have to create handler, remember handler, passed this specific handler to operator+=). Providing an extra non-operator-overload method could be a trade off, allow both purity of the interface (one who is willing to use += will use it, one who is lazy, will use "subscribe" method).

This API comes from Events Tutorial.

I disagree with having a .subscribe method. There should be only one way to do a thing.

Saving a EventHandler isn't more difficult than saving an opaque id.

If you are talking about EventEmitter<Interest>::Handler being too long, I can change it as:

typedef EventHandler<Interest> InterestEventHandler;
class Face
{
public:
  Event<InterestEventHandler> onReceiveInterest;
};

void
handleReceiveInterest(const Interest& interest)
{
}

void
subscribe()
{
  Face face;
  InterestEventHandler handler = makeEventHandler(&handleReceiveInterest);
  face.onReceiveInterest += handler;
  face.onReceiveInterest -= handler;
}

Then, EventEmitter<TArgs> is mapped to Event<EventHandler<TArgs>>.

This shall be cleaner, and closer to C# syntax.

What about middle ground. operator+= can have arbitrary return type (I checked with clang and gcc). So we can have something like

template<typename ...TArgs>
class EventEmitter : noncopyable
{
public:
  typedef function<void(const TArgs&...)> EventDelegate;
  typedef shared_ptr<EventDelegate> Handler;

  Handler
  operator+=(const EventDelegate& handler);

  Handler
  operator+=(Handler handler);

  void
  operator-=(Handler handler);

  ...

private:
  std::vector<Handler> m_handlers;
};

and one would be able to write

void
subscribe()
{
  Face face;
  auto handler = (face.onReceiveInterest += &handleReceiveInterest);
}

Why? Caller is supposed to explicitly create the Handler. I don't want to save this one line of code.

This syntax would allow either case. I don't like creating handler explicitly - function pointer is a handler. Whatever is returned by the += is internal business of the event emitter and I don't want to deal with that explicitly.

In fact, += shall return void.

Function pointer is not a handler because it's not comparable for equality.

Junxiao Shi wrote:

In fact, += shall return void.

This is not a fact. I want it to return "handler" that can be used to "remove" the event from the scheduler. I don't want to explicitly create anything except std::function

Function pointer is not a handler because it's not comparable for equality.

This is not the point. EventEmitter should not force me to do some other magic except standard bind()/lambda/function pointer arithmetics. If needed, the operation just need to return and the caller to remember some opaque identifier. This can be even index of std::vector (or iterator of std::list) that could be used to remove the item. The point is "opaque id". It doesn't need to be the handler.

Exactly the same thing is already happening with expressInterest, registerPrefix, and setInterestFilter. I want EventEmitter to allow the same usage pattern.

Alex Afanasyev wrote:

Junxiao Shi wrote:

In fact, += shall return void.

This is not a fact. I want it to return "handler" that can be used to "remove" the event from the scheduler. I don't want to explicitly create anything except std::function

Although it is true that the C++ language doesn't enforce any particular return type for operator+=, it is customary to return X& when the operator overload is a member of a class X. Returning something else would feel unnatural. I suggest to simply use a regular method call, why is it needed to overload an operator?

I agree with Alex on pretty much everything else.

I disagree with using a regular function call.
EventEmitter is designed to use C# convention, so += and -= should be used.

There should be only one way to subscribe and unsubscribe: explicitly create a Handler.

I don't understand why following C# API conventions is a design goal now... for a C++ project...

I don't understand why following C# API conventions is a design goal now... for a C++ project...

It's a design goal for original EventEmitter, and I intend to keep it.

This Task cannot progress because there's no consensus on API.

The requirement of explicitly calling makeEventHandler is to allow sharing EventHandler objects.

I could add a helper function as a free function:

EventSubscription&&
subscribe(EventEmitter<...> ee, std::function<void(...)> f);

EventSubscription is an opaque object. It is movable but not copyable. When it's destructed, the subscription is unsubscribed.

Therefore, this object can be assigned to a class member field, and caller doesn't need to explicitly unsubscribe in the destructor.

I'm fine either way. My only remark is that I don't think that using a C# API to model a C++ class makes much sense. Especially when X::operator+= starts returning things that are not X (or references to X). It just feels very unnatural for a C++ programmer.

Especially when X::operator+= starts returning things that are not X (or references to X). It just feels very unnatural for a C++ programmer.

No, operator+= won't return other things.

20141128 conference call agrees on the following API, given as examples:

class Face
{
public:
  EventEmitter<Interest> onReceiveInterest;
};

class MySubscriber1
{
public:
  explicit
  MySubscriber1(Face& face)
  {
    m_faceReceiveInterestSubscription = face.onReceiveInterest.subscribe(bind(&MySubscriber1::processInterest, this, _1));
  }

private:
  void
  processInterest(const Interest& interest);

private:
  EventAutoCancel m_faceReceiveInterestSubscription;
  // subscription is automatically cancelled when this object is destructed;
  // old subscription is automatically cancelled when a new event subscription is assigned to it
};

class MySubscriber2
{
public:
  explicit
  MySubscriber2(Face& face)
  {
    m_faceReceiveInterestSubscription = face.onReceiveInterest.subscribe(bind(&MySubscriber2::processInterest, this, _1));
  }

  void
  cancel()
  {
    m_faceReceiveInterestSubscription.reset();
  }

private:
  void
  processInterest(const Interest& interest);

private:
  EventSubscription m_faceReceiveInterestSubscription; // cancellation requires an explicit .cancel() call
};

class MySubscriber3
{
public:
  explicit
  MySubscriber3(Face& face)
  {
    // when return value of .subscribe is ignored, the subscription cannot be cancelled
    face.onReceiveInterest.subscribe(bind(&MySubscriber3::processInterest, this, _1));
  }

private:
  void
  processInterest(const Interest& interest);
};

If EventEmitter is destructed, all subscriptions are cancelled, and further automatic or manual cancellations have no effect.

operator+= is deprecated.

Works for me... but, before reinventing the wheel, have you considered using an established API (and implementation) such as Boost.Signals2?

Boost.Signals2 seems powerful.

Now the choice becomes:

• implement EventEmitter API using Boost.Signals
• deprecate EventEmitter entirely, and change everything to use Boost.Signals2

I remember we were considering boost::signal2 before and the main reason was (and it still holds, at least for me) performance. According to this https://testbit.eu/cpp11-signal-system-performance/, boost::signal2 has somewhat big overhead compared to simple implementations (and I hope our implementation falls into this category).

I also say that using boost::signal2 would be overkill for us, at least at this stage and would vote for just extending the existing EventEmitter.

However, I will be perfectly ok with keeping the proposed naming or moving closer to boost::signal2 naming conventions:

Connection e = EventEmitter::connect(...)
e.disconnect();

{
  ScopedConnection e = EventEmitter::connect(...)
  // e
}

Boost.Signals2 by default is thread-safe so it uses mutexes.
I expect the performance to be much better when dummy_mutex is picked.

I understand that, but without additional benchmarking, I would be conservative in enabling use of boost::signal2 instead of our simple EventEmitter. Signal2 have more functionality, including return value aggregation (unless I'm mis-interpreting it), which may have some impact on performance, beside locking.

Another "negative" issue with boost::signal2 is code size. I got some benchmarks here https://code.google.com/p/nano-signal-slot/wiki/Performance#Runtime, though I'm not sure how representative they are.

Alex Afanasyev wrote:

Another "negative" issue with boost::signal2 is code size. I got some benchmarks here https://code.google.com/p/nano-signal-slot/wiki/Performance#Runtime, though I'm not sure how representative they are.

Interesting... the above benchmark (together with the raw numbers here https://github.com/NoAvailableAlias/nano-signal-slot/tree/master/benchmark#performance) seems to give conflicting results compared to the benchmark in comment #28.

Also, for our use cases code size is a minor problem compared to signal emission speed.

I'll run a simple benchmark of Boost.Signals2 with regular mutex vs dummy mutex.

I performed a benchmark:

#include <ndn-cxx/util/time.hpp>
#include <ndn-cxx/util/event-emitter.hpp>
#include <boost/signals2.hpp>

int g_sum;

void
f(int n)
{
  g_sum += n;
}

int
main()
{
  g_sum = 0;
  ndn::util::EventEmitter<int> ee;
  ee += &f;

  auto t1 = ndn::time::system_clock::now();
  for (int i = 0; i <= 99999999; ++i) {
    ee(1);
  }
  auto t2 = ndn::time::system_clock::now();
  std::cout << g_sum << " " << (t2 - t1) << std::endl;

  g_sum = 0;
  boost::signals2::signal<void(int)> sig;
  sig.connect(&f);

  auto t3 = ndn::time::system_clock::now();
  for (int i = 0; i <= 99999999; ++i) {
    sig(1);
  }
  auto t4 = ndn::time::system_clock::now();
  std::cout << g_sum << " " << (t4 - t3) << std::endl;

  g_sum = 0;
  namespace bs2 = boost::signals2;
  bs2::signal_type<void(int), bs2::keywords::mutex_type<bs2::dummy_mutex>>::type sig2;
  sig2.connect(&f);

  auto t5 = ndn::time::system_clock::now();
  for (int i = 0; i <= 99999999; ++i) {
    sig2(1);
  }
  auto t6 = ndn::time::system_clock::now();
  std::cout << g_sum << " " << (t6 - t5) << std::endl;

  return 0;
}

Boost.Signals2 is considerably slower than ndn-cxx EventEmitter (commit e04bd8339110ff45a58612f4329c7ec504a45580).

On Ubuntu, 1088 events can be emitted per millisecond with Boost.Signals2 with dummy_index.
If NFD is processing 10879 packets per second (a reasonable goal), 1% of CPU time would be spent in event emission.
This is unacceptable.

We shouldn't replace EventEmitter with Boost.Signals2.

I recall that note-33 benchmark was done with debug mode. The result from release build would lead to same conclusion:

A controversial change in commit 7d8644a52162b0f2ac53825ebb97d91e53f71b2b is: during event triggering, the changes to subscription list won't take effect until all handlers have been executed.

Conceptually, EventEmitter<TArgs...>::operator():

1. makes a snapshot of the subscription list before executing any handler
2. invokes each handler in the snapshot; order of invocation is not guaranteed
3. if a handler adds or removes subscription, those changes have no effect on the current event emission, but will take effect next time the event is emitted

Since EventEmitter doesn't support specifying the order of handler invocation, and doesn't guarantee the order of invocation,
if we allow changes to take effect immediately,
when handler1 removes the subscription of handler2, it would be unpredictable whether handler2 would be invoked during the current event emission.

Thus, I decide that all handlers should be invoked for the current event emission.

To make the semantics consistent among all subscription list changes (add, remove, clear), all changes will not take effect for the current event emission.

To simplify implementation, if multiple conflicting changes are performed (eg. add handler3 then remove handler3; clear then add), the result would be an unpredictable but valid state.
I believe that multiple conflicting changes during an event emission is rare enough to consider for.

So let's take a step back.

Is there a use case for modifying the list of handlers while a handler is executing? If so, which operations should be supported?

1. The currently executing handler wants to remove itself. This is fine, doesn't raise any semantics ambiguities, and should be easy to implement.

2. The currently executing handler wants to remove another handler, different from itself. I don't think this is needed.

3. A handler adds a new subscription. This looks fine and can be done with the current "delayed" logic.

4. A handler clears the whole list of handlers. This should not be supported in my opinion.

I basically agree with the semantics described in note-38.

However, what's the harm if removing another handler and clearing are allowed?

After more thinking, I am now objecting "subscribe/unsubscribe" naming. boost::signal, boost::signal2, QT signals, libsig++, NS-3, and suspect of tons of other signal libraries use "connect/disconnect", terminology. We should use the same terminology for the same type of abstractions.

Regarding naming:

This feature is called EventEmitter, and there's no reason to change this.

EventEmitter emits an event, not a signal.

Other types subscribe or listen or hook up to an event, not connect to an event.

Another problem is how to enforce only the declaring class can raise / trigger the event.

I plan to make .subscribe a const method, so that when such enforcement is desired, a const reference can be exposed.

Note that the const reference should be exposed as a field, not as a method, so that the syntax looks the same; having a .subscribeToX method is also unacceptable.

class Face
{
protected:
  Face()
    : onReceiveInterest(m_onReceiveInterest)
  {
  }
protected:
  EventEmitter<Interest> m_onReceiveInterest; // trigger the event using this object
public:
  const EventEmitter<Interest>& onReceiveInterest; // subscribe to the event on this reference
};

In most cases the convention is sufficient.

This technique is needed only when additional protection is desired, such as #2272.

Junxiao Shi wrote:

However, what's the harm if removing another handler and clearing are allowed?

They substantially complicate the semantics, and if we don't have any clear use case for these operations in mind, we may end up implementing a behavior that we will regret later. Also, not giving any guarantee about what happens basically makes the functionality useless in most cases. So unless it's needed, don't allow it for now, we can always revisit and enable it later with more precise semantics.

I kind of like the connect/disconnect API naming better too, but Junxiao has a point: the current class is called EventEmitter, we would have to rename that one too (e.g. to Signal).

Given we are doing the refactoring, may be it is ok to rename. It just a little weird that we use different terminology from everybody else.