[C++] Looking for feedback on my event system

Started by
44 comments, last by _the_phantom_ 12 years, 7 months ago
I've not forgotten about this and did come up with a half-solution today (I say half-solution because it currently lacks an unregister chunk of functionality) which seems to perform as well as your initial solution in high listener/low message count situations. It does seem to scale less well as message counts increase, which I find intresting however as I'm seeing strange data (some timings of 0ms) and the memory leaks are killing my longer test cases to get better min/max/average timing, I need to poke at it some more before commiting myself to anything.

The code itself is pretty simply however;

[source]
class BaseEventDispatcher
{

};

template<typename T>
class SpecificEventDispatcher : public BaseEventDispatcher
{
typedef std::function<void(T*)> ListenerCallback;
typedef std::vector<ListenerCallback> ListenerVector;

public:
void RegisterCallBack(ListenerCallback callback)
{
listeners.push_back(callback);
}

void DispatchEvent(T &event)
{
T* eventptr = &event;
std::for_each(listeners.begin(), listeners.end(),
[eventptr](ListenerCallback &callback)
{
callback(eventptr);
});
}

private:
ListenerVector listeners;
};

class EventDispatcher
{
typedef std::map<int, BaseEventDispatcher*> ListenerMap;
public:
EventDispatcher() {}
virtual ~EventDispatcher() {}

template<typename ClassT>
void Register(typename SpecificEventDispatcher<ClassT>::ListenerCallback callback);

template<typename ClassT>
void Dispatch(ClassT *event);

template<typename ClassT>
void Dispatch(ClassT &event);

private:
ListenerMap listeners_;
};

template<typename ClassT>
void EventDispatcher::Register(typename SpecificEventDispatcher<ClassT>::ListenerCallback callback )
{
ListenerMap::iterator itr = listeners_.find(UniqueTypeID<ClassT>::kID);
if( itr == listeners_.end())
{
auto * sed = new SpecificEventDispatcher<ClassT>();
sed->RegisterCallBack(callback);
listeners_.insert(std::make_pair(UniqueTypeID<ClassT>::kID, sed));
}
else
{
SpecificEventDispatcher<ClassT> *sed = static_cast<SpecificEventDispatcher<ClassT> *>(itr->second);
sed->RegisterCallBack(callback);
}
}

template<typename ClassT>
void EventDispatcher::Dispatch(ClassT &event)
{
ListenerMap::iterator itr = listeners_.find(UniqueTypeID<ClassT>::kID);
if( itr != listeners_.end())
{
SpecificEventDispatcher<ClassT> *sed = static_cast<SpecificEventDispatcher<ClassT> *>(itr->second);
sed->DispatchEvent(event);
}
}

template<typename ClassT>
void EventDispatcher::Dispatch(ClassT *event)
{
ListenerMap::iterator itr = listeners_.find(UniqueTypeID<ClassT>::kID);
if( itr != listeners_.end())
{
SpecificEventDispatcher<ClassT> *sed = static_cast<SpecificEventDispatcher<ClassT> *>(itr->second);
sed->DispatchEvent(*event);
}
}

[/source]

And the test code as it stands;

[source]
Utils::Timer registerTimer;
size_t foo = 0;
PhantomVersion::EventDispatcher eventDispatcher;
registerTimer.Start();
for(size_t i = 0; i < listeners; ++i)
{
switch (dispatchTypes)
{
case 0:
eventDispatcher.Register<TestEvent1>(std::bind(&TestListener::OnEvent, new TestListener(), std::placeholders::_1));
break;
case 1:
eventDispatcher.Register<TestEvent3>(std::bind(&TestListener2::OnEvent, new TestListener2(), std::placeholders::_1));
break;
case 2:
eventDispatcher.Register<TestEvent4>(std::bind(&TestListener3::OnEvent, new TestListener3(), std::placeholders::_1));
break;
case 3:
eventDispatcher.Register<TestEvent5>(std::bind(&TestListener4::OnEvent, new TestListener4(), std::placeholders::_1));
break;
}

};
registerTimer.Stop();
times.registerTest = registerTimer.GetMilliseconds();

registerTimer.Start();
for(size_t i = 0; i < events; ++i)
{
switch(eventTypes)
{
case 0:
{
TestEvent1 *bar = new TestEvent1(1,2,3);
eventDispatcher.Dispatch(bar);
foo += bar->x_;
delete bar;
}
break;
case 1:
{
TestEvent3 *bar = new TestEvent3(1,2,3);
eventDispatcher.Dispatch(bar);
foo += bar->x_;
delete bar;
}
break;
case 2:
{
TestEvent4 *bar = new TestEvent4(1,2,3);
eventDispatcher.Dispatch(bar);
foo += bar->x_;
delete bar;
}
break;
case 3:
{
TestEvent5 *bar = new TestEvent5(1,2,3);
eventDispatcher.Dispatch(bar);
foo += bar->x_;
delete bar;
}
break;
}

}
registerTimer.Stop();
times.ptrTest = registerTimer.GetMilliseconds();

registerTimer.Start();
for(size_t i = 0; i < events; ++i)
{
switch(eventTypes)
{
case 0:
{
TestEvent1 bar(1,2,3);
eventDispatcher.Dispatch(bar);
foo += bar.x_;
}
break;
case 1:
{
TestEvent3 bar(1,2,3);
eventDispatcher.Dispatch(bar);
foo += bar.x_;
}
break;
case 2:
{
TestEvent4 bar(1,2,3);
eventDispatcher.Dispatch(bar);
foo += bar.x_;
}
break;
case 3:
{
TestEvent5 bar(1,2,3);
eventDispatcher.Dispatch(bar);
foo += bar.x_;
}
break;
}

}
registerTimer.Stop();
times.refTest = registerTimer.GetMilliseconds();
[/source]



Advertisement
I like this version, but for me it is the slowest one of the four... Maybe I implemented something wrong. I'll have to do some testing.
I think in my own testing the above solution didn't perform well in situations where you have a single event type, few listeners and many events.

However during testing it dawned on me that, as single point benchmarks go, this method of testing isn't very representative; in a typical message system you'll have more than one type of event, many listeners and a few events per frame being triggered.

Thus my 'test' function above which does a 'random' dispatch of types to a 'random' mix of listeners registered to the events; in this more typical situation the above solution could match the initial one (I was unable to test the variadic template version as VS2010 doesn't support it), at least with my slightly wonky timing data. ('random' data here is created by seeding the RNG with '5' each run to get the same output and thus same test data)

There is also the 'dispatch' problem which isn't taken into account; your test method plays well because it gets great cache reuse, however in a typical game situation the recieving code might well do more and more importantly from the pattern you've given for usage of a direct send->handle the rest of the app will be doing other things as well in between calling the 'dispatch' method.

Really a 'true' benchmark needs to take both a 'real' usage pattern into account, including the various cache invalidations etc which go on, as currently imo your results depend heavily on the cache being warm at dispatch time.(I tried a quick way of dealing with this by allocating 8meg of memory and writing random numbers to it between tests, however this might not help the situation as much as I hope).
So, it turns out that I'm an idiot... <_<

If you notice in the test above I'm using event objects based off your original Event class which inherits from EventListener_Interface which defines a function 'virtual void OnEvent(EventT * event)' which is what the code above binds.

Can you see what I did wrong?

Yep, I'm binding to a virtual function which means I'm paying the cost of a virtual lookup still with the added overhead of the rest of the code :(

Creating a new set of events in a 'nonVirtual' namespace which don't inherit from any base classes and just have an OnEvent function for each Event type and bind those instead yields a system which has an execution time which seems like it matches the original code when being used with 500 listeners and 50000 events.

I seem to have some timing problems still as the original code shows a max runtime of 4seconds which is clearly insane when compared to the min case so I'll attack the test code at some point to figure out wtf I've done wrong :D

For now however these are the events I'm using to bind the above code to now (basically prefix all the Listener classes with nonVirtual:: and change TestListener to TestListener1);

[source]
namespace nonVirtual
{
class TestListener1
{
public:
TestListener1() {};

void OnEvent(TestEvent1* event)
{
event->x_ = event->x_*event->x_ +
event->y_*event->y_ +
event->z_*event->z_;
}
};

class TestListener2
{
public:
TestListener2() {};

void OnEvent(TestEvent3* event)
{
event->x_ = event->x_*event->x_ +
event->y_*event->y_ +
event->z_*event->z_;
}
};

class TestListener3
{
public:
TestListener3() {};

void OnEvent(TestEvent4* event)
{
event->x_ = event->x_*event->y_ +
event->y_*event->z_ +
event->z_*event->x_;
}
};

class TestListener4
{
public:
TestListener4() {};

void OnEvent(TestEvent5* event)
{
event->x_ = event->x_*event->z_ +
event->y_*event->x_ +
event->z_*event->y_;
}
};
}
[/source]


I have recently reworked by event system completely and it now looks totally different.
First of all I did the same design flaw that you can find in many solutions. There is something like

void fireEvent(EventData* evn) {
foreach ( Listener l : listeners ) {
onEvent(evn);
}
}


First using a raw pointer here is awfull. But the major problem is that every event will be processed immediately when they are fired.
This leads to unpredictable circumstances in your code. You actually never know WHEN an event will be fired and therefore you cannot
know the states of your objects.
So what I did was to buffer the events and then at a certain point in my engine (right after the big update loop) I process ALL events and
send them to the listeners.
Also in my opinion using a base class for an event is awfull. An event contains only data. You are just sending plain data around.
So I removed this completely and just using structs. This makes the code much leaner and cleaner.
The entire solution now looks completely different.
I got the idea after reading this:
http://bitsquid.blogspot.com/2011/02/managing-decoupling-part-2-polling.html
It took me quite a while to finally get it and then it just hit me how awfully right the author is.

Actually I now have a buffer where I store the events like:
[header1][event data][header2][event data2]
The header is a simple struct:


struct Header {
uint32 index;
uint32 id;
size_t size;
};


The ID is the actual ID of the event and the size is the size of the event data struct. The index is the actual index in my buffer.
The entire buffer is sent to any listener and the listener can grab any data he likes from the buffer like:

size_t get(void* p, size_t size,uint32 index);


Example:
Here is the event data when the ship is hit by a projectile:

struct ShipHit {
Vec2 pos;
int damage;
int rocketType;
}


Now I fire the event:

ShipHit sh;
sh.pos = Vec2(100,100);
sh.rocketType = 1;
sh.damage = 100;
evnetManager->fireEvent(100,&sh,sizeof(sh);


In the listener I do something like:

// check if we have a ship hit event in the buffer
if ( buffer.containsID(100) ) {
// yes so grab the data
ShipHit sh;
buffer.get(100,&sh,sizeof(sh));
}



I hope I could actually shed some light on my approach. It is actually not easy and I am not sure I could get the point across.
Just ask if you have any questions.
Deferred event execution isn't always desirable. It depends on which systems are producing and consuming the events. Plus you know exactly when events will be fired -- whenever [font="Courier New"]fireEvent[/font] is called.
[font="arial, verdana, tahoma, sans-serif"]
[font="arial, verdana, tahoma, sans-serif"]

Yep, I'm binding to a virtual function which means I'm paying the cost of a virtual lookup still with the added overhead of the rest of the code :(

Ah, I will have to rework my tests a bit as well. Good catch.[/font]

[/font]

I have recently reworked by event system completely and it now looks totally different.
First of all I did the same design flaw that you can find in many solutions. There is something like

void fireEvent(EventData* evn) {
foreach ( Listener l : listeners ) {
onEvent(evn);
}
}




First using a raw pointer here is awfull. But the major problem is that every event will be processed immediately when they are fired.
This leads to unpredictable circumstances in your code. You actually never know WHEN an event will be fired and therefore you cannot
know the states of your objects.

I agree and disagree here. I think Zipster summed it up well:

Deferred event execution isn't always desirable. It depends on which systems are producing and consuming the events. Plus you know exactly when events will be fired -- whenever [font="Courier New"]fireEvent[/font] is called.

This is why I plan on adding the ability to push and poll events along side immediately firing events (i.e. PushEvent(blah), PollEvents(blah), FireEvent(blah)).

Also, all of the versions seen here are test versions and have very basic functionality. These tests have been created for the soul purpose of seeing how long it will take to dispatch a high number of events to a medium-high number of listeners. In other words, what PollEvent would eventually do.



Also in my opinion using a base class for an event is awfull. An event contains only data. You are just sending plain data around.
So I removed this completely and just using structs. This makes the code much leaner and cleaner.

I am beginning to come around to this school of thought as well. Creating events is becoming tedious as I am simply adding getters for each member variable I add. I will probably change over to structures eventually and use class events when I need more than just plain old data. My system can already handle both, so the change should just be a bit of refactoring work.



I hope I could actually shed some light on my approach. It is actually not easy and I am not sure I could get the point across.
Just ask if you have any questions.

Thanks. I've had the plan to add/change some of the features you talk about. And you've been another voice on top of a couple others' saying I should use structs for passing around events, which may finally make me do it wink.gif.

Deferred event execution isn't always desirable. It depends on which systems are producing and consuming the events. Plus you know exactly when events will be fired -- whenever [font="Courier New"]fireEvent[/font] is called.


Indeed, in a highly parrallel system a deferred event execution simplifies things greatly, although it does introduce a frame of latency on acting on those messages.

But as they say; horses for courses... if you don't need such a system then such a system isn't a "better" choice.

(Personally I probably would use it but then I tend towards lock-less task parallel designs which this method suits nicely)
I'm now switched over to a design very similar to the one posted by phantom, with my own modifications here and there, but have run into a problem: unregistering listeners. Before it was as simple as iterating through a vector of listeners and removing the object that was the same as the one passed into the function. Now that is not possible because std::functions aren't comparable. I've a few ideas on how to solve this, but they add some annoying layer of complexity that I really don't want. Any ideas?
The only solution I've come up with is the one I presented in another thread where instead of a vector of listeners you use a list and store iterators of added listens into another container which the registering class instance holds.

However this solution relied on the functors having the same interface/signature so the could be stored :-/

This topic is closed to new replies.

Advertisement