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

Started by
44 comments, last by _the_phantom_ 12 years, 6 months ago

Ok, although I don't use std::function too often, I noticed some things to improve:
- you are leaking memory
- why don't you just let users register a std::function object directly?

Where? You mean in the test code? If so, I know, I was just testing so I didn't care smile.gif. If the dispatcher is leaking memory, however, that is another story.



In addition, you could hide some boilerplate (event id , event base ) with this technique:


unsigned int next_id()
{
unsigned int previous_id = 0;
return ++previous_id; //or return previous_id++ , depending on wheter you need a NULL value
}
template< class Type >
class type_id
{
public:
static unsigned int value;
};
template< class Type >
unsigned int type_id< Type >::value = next_id(); //unique value for each type


Indeed! Very nice, that is a much better way of doing it. I salute you sir.



To save the map lookup, you could use a vector whose size is as big as the number of EventType s you have instead of a map.
(to do is, check in register and dispatch whether your vector is large enough and resize it if needed).
Then you can just access the listener_vector for any type T with

listener_vectors[ type_id< T >::value ];

which has O(1), but probably consumes more memory if you have multiple dispatchers.

I thought of this, but was unsure if I should do it this way or not. It would surely consume more memory, but hopefully to a lesser extent if I use a vector of vector pointers and set the thus far unused ones to NULL until they are needed for the first time.


Advertisement
You have some std::vector< void* > s. You fill them with 'new std::function<...>'. Where do you delete this memory?
A solution would be to have something like a

template< class EventType >
class listener_vector
: public std::vector< std::function< void( const EventType & ) > > ,
public virtual_base //has virtual destructor
{
};

and store a vector< virtual_base* > instead.
Concerning the memory issue: You could make the dispatcher a singleton (although I dislike singletons). If there is just one instance of the dispatcher, there won't be a memory overhead when using vector instead of map. Since dispatching is independent from the number of events, it won't hurt your performance.
I've not had a chance to look closely at the code however what might be hurting you here is the dispatcher having to go via a void pointer for every intrested party.

You can probably solve this by introducing a class which holds std::function/boost::function objects for a certain type and using that in your primary map and then having that class perform the dispatch.

Sure, it adds another layer BUT I suspect this would be less painful than doing multiple casts from void to the correct type at dispatch time (it would certainly help the compiler).

I'll take a closer look once I get home and see if I can whip up some psuedo code to demo what I mean if you haven't already come up with something :)

You have some std::vector< void* > s. You fill them with 'new std::function<...>'. Where do you delete this memory?

Ah, yes. This was just a quick version I made up for testing. I'm first getting the design I want and testing it for efficiency, then going back and adding in all the necessary parts, such as deleting those, once I figure out which I plan on using. Currently I have 5 different versions of this class, so I'm skipping a lot of steps unsure.gif.




Concerning the memory issue: You could make the dispatcher a singleton (although I dislike singletons). If there is just one instance of the dispatcher, there won't be a memory overhead when using vector instead of map. Since dispatching is independent from the number of events, it won't hurt your performance.

I thought about this as well, but I feel the same way about singletons. A class with static members and functions would work the same way, but I'm not sure I'm ready to go the completely centralized messaging system route just yet. It may actually counteract some of the reasons I'm switching to a delegate pattern. For instance, if I have a button class that inherits or uses an event dispatcher to dispatch a ButtonDownEvent (or whatever) I can register specific functions to that button (e.g. void OnMenuButtonDown(const ButtonDownEvent&) registered to Button menu_button) and messages would get dispatched to only that button's functions, saving a good bit of time. If I use a central messaging system, I would have to register functions to the general ID for ButtonDownEvent. Then, whenever any button was pressed, all functions registered to that event type would get the message and each would have to have a filter for the correct button. A way around that would be to alter my design and add some kind of pairing between the event type and its sender, which wouldn't be too difficult, but it would increase the number of registered event types from 1 (UniqueTypeID<ButtonDownEvent>::kID) to the amount of objects created with registered functions, which would probably be all of them.

I've not had a chance to look closely at the code however what might be hurting you here is the dispatcher having to go via a void pointer for every intrested party.

You can probably solve this by introducing a class which holds std::function/boost::function objects for a certain type and using that in your primary map and then having that class perform the dispatch.

Sure, it adds another layer BUT I suspect this would be less painful than doing multiple casts from void to the correct type at dispatch time (it would certainly help the compiler).

I'll take a closer look once I get home and see if I can whip up some psuedo code to demo what I mean if you haven't already come up with something :)

I would appreciate it laugh.gif. I'll post the one that is running fastest for me at the moment.

// NOTE: Test version. Needs a lot of clean up code and extra features.
class EventDispatcher {
public:
// typedefs: ===============================================================
typedef std::function<void(inp::EventBase*)> ListenerCallback;
typedef std::vector<ListenerCallback> ListenerVector;
typedef std::map<int, ListenerVector> ListenerMap;

// ctors/dtor: =============================================================
EventDispatcher() {}
virtual ~EventDispatcher() {}

// public functions: =======================================================
// - listeners_ access
void Register(int type, void (*func)(inp::EventBase*));

template<typename ClassT>
void Register(int type, void (ClassT::*func)(inp::EventBase*), ClassT* obj);

// - dispatch event
void Dispatch(int type, inp::EventBase* event);

private:
// vars: ===================================================================
ListenerMap listeners_;
};

// public functions ============================================================
// - listeners access ----------------------------------------------------------
void EventDispatcher::Register(int type, void (*func)(inp::EventBase*)) {
listeners_[type].push_back(ListenerCallback(func));
}

template<typename ClassT>
void EventDispatcher::Register(int type, void (ClassT::*func)(inp::EventBase*), ClassT* obj) {
listeners_[type].push_back(
ListenerCallback(std::bind(func, obj, std::placeholders::_1)));
}

// - dispatch event ------------------------------------------------------------
void EventDispatcher::Dispatch(int type, inp::EventBase* event) {
if (event == NULL) { return; } // disallow a NULL event

ListenerMap::iterator itr = listeners_.find(type);
if (itr == listeners_.end()) { return; } // type not found

// dispatch event to the listeners
for (unsigned int x = 0; x < itr->second.size(); ++x) {
itr->second.at(x)(event);
}
}

It seems that by passing a function with the EventBase as a parameter and then typecasting in that function I can increase speed a bit, but it is not necessarily the way I want to do it.

[s]Also, how do you +1 people these days? I have people I must give credit to, such as GorbGorb, and can't seem to find it ><.[/s] Found it.
I'm not sure why you need to have exactly one dispatcher for all possible event types. You mentioned previously that you intentionally avoided that pattern, but surely this is where you're getting hit in performance, since as phantom mentioned it has to cast to the correct type for every invocation, on top of an extra pointer dereference because of the additional void* indirection. If you were to template the dispatcher itself, then then the guts become a lot simpler and you can store ::function's directly, with no casting, pointers, or map look-ups. Something like this (disclaimer: typed directly into post):


template<typename EventT>
class EventDispatcher
{
public:
typedef std::function<void(EventT*)> ListenerCallback;
typedef std::vector<ListenerCallback> ListenerVector;

void Register(ListenerCallback callback);
void Unregister(ListenerCallback callback);
void Dispatch(EventT* event);
};

This is pretty much what Antheus posted, plus it's essentially how C# events work. If you're adamantly opposed to this pattern then that's your prerogative I guess, but it's how I've seen it done many times before and it works very well :)

I'm not sure why you need to have exactly one dispatcher for all possible event types. You mentioned previously that you intentionally avoided that pattern, but surely this is where you're getting hit in performance, since as phantom mentioned it has to cast to the correct type for every invocation, on top of an extra pointer dereference because of the additional void* indirection. If you were to template the dispatcher itself, then then the guts become a lot simpler and you can store ::function's directly, with no casting, pointers, or map look-ups. Something like this (disclaimer: typed directly into post):


template<typename EventT>
class EventDispatcher
{
public:
typedef std::function<void(EventT*)> ListenerCallback;
typedef std::vector<ListenerCallback> ListenerVector;

void Register(ListenerCallback callback);
void Unregister(ListenerCallback callback);
void Dispatch(EventT* event);
};

This is pretty much what Antheus posted, plus it's essentially how C# events work. If you're adamantly opposed to this pattern then that's your prerogative I guess, but it's how I've seen it done many times before and it works very well :)

Actually, this is one of the versions of the dispatcher I'm testing and it is, strangely enough, dispatching slower than the version I previously posted. In the same test I mentioned earlier it is performing an average of ~70 ms worse than the map-based event dispatcher. I'm not quite sure why.

Code:

template<typename EventT>
class SpecificEventDispatcher {
public:
// typedefs: ===============================================================
typedef std::function<void(EventT*)> ListenerCallback;
typedef std::vector<ListenerCallback> ListenerVector;

// ctors/dtor: =============================================================
SpecificEventDispatcher() {}
virtual ~SpecificEventDispatcher() {}

// public functions: =======================================================
// - listeners_ access
void Register(void (*func)(EventT*));

template<typename ClassT>
void Register(void (ClassT::*func)(EventT*), ClassT* obj);

// - dispatch event
void Dispatch(EventT* event);

private:
// vars: ===================================================================
ListenerVector listeners_;
};

template<typename EventT>
void SpecificEventDispatcher<EventT>::Register(void (*func)(EventT*)) {
listeners_.push_back(func);
}

template<typename EventT> template<typename ClassT>
void SpecificEventDispatcher<EventT>::Register(void (ClassT::*func)(EventT*),
ClassT* obj) {
listeners_.push_back(std::bind(func, obj, std::placeholders::_1));
}

// - dispatch event
template<typename EventT>
void SpecificEventDispatcher<EventT>::Dispatch(EventT* event) {
if (event == NULL) { return; } // disallow a NULL event

// dispatch event to the listeners
for (unsigned int x = 0; x < listeners_.size(); ++x) {
listeners_.at(x)(event);
}
}

So either my tests are flawed, or this causes something to happen that I'm not expecting.
I can't see a reason for worse performance (are you sure about that?), but I have a few things to improve it for both versions:

if (event == NULL) { return; } // disallow a NULL event

If you pass a const reference instead of a pointer, you do not have to check for NULL.

for (unsigned int x = 0; x < listeners_.size(); ++x) {
listeners_.at(x)(event);

should be

for( auto it = listeners_.begin() ; it != listeners_.end() ; ++it )
(*it)( event );

If you use iterators (and a vector iterator is actually just a pointer) you'll save one addition in each loop and your code works for every container.
By the way, operator[] is faster than at(), because at() checks for invalid indexes while operator[] doesn't (and I don't really see a point in throwing an out_of_range exception. When will you catch it and what will you do to recover?).
Ah before I forget it: static_cast and reinterpret_cast are 'executed' at compile time. You won't see them in a binary. While it may make some optimizations harder, I doubt it in this case because you are performing it on heap memory anyway. Most compilers don't optimize as much when objects are on the heap.
So, Space Marine happened which means I didn't get a chance to look at this today...

Shadeirou; is all the code for your tests in this thread? If so I'll see about knocking up my own tests based on it... if not could you post it somewhere :)

So, Space Marine happened which means I didn't get a chance to look at this today...

Shadeirou; is all the code for your tests in this thread? If so I'll see about knocking up my own tests based on it... if not could you post it somewhere :)

It is, but I'll post it here again. Or you can download all the files I'm testing here. Please forgive the file structure and naming. I don't tend to organize until after I'm done testing.

// everything is defined in separate files, but I include them here for simplicity.

// eventbase.h -----------------------------------------------------------------
namespace inp {
class EventBase {
public:
virtual ~EventBase() {}
};
} // namespace inp


// testlistener.h --------------------------------------------------------------
class TestEvent1 : public inp::EventBase {
public:
TestEvent1(int x, int y, int z) : x_(x), y_(y), z_(z) {}

int x_, y_, z_;
};

class TestEvent2 : public inp::EventBase {
public:
TestEvent2(const std::string& str) : str_(str) {}

std::string str_;
};

class TestListener : public inp::EventListener_Interface<TestEvent1> {
public:
TestListener();

void OnEvent(TestEvent1* event);
void OnTestEvent1(TestEvent1* event);
void OnTestEvent1base(inp::EventBase* event);
};

// testlistener.cpp

TestListener::TestListener() {}

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

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

void TestListener::OnTestEvent1base(inp::EventBase* event) {
TestEvent1* test = static_cast<TestEvent1*>(event);
test->x_ = test->x_*test->x_ +
test->y_*test->y_ +
test->z_*test->z_;
}


// main.cpp --------------------------------------------------------------------
int main() {
int listeners = 500, events = 50000;
lib::Timer timer;

// TEST FUNCTION-BASED EVENTDISPATCHER ---------------------------------------
EventDispatcher func_disp;

timer.Start();
for (int x = 0; x < listeners; ++x) {
func_disp.Register(lib::UniqueTypeID<TestEvent1>::kID,
&TestListener::OnTestEvent1base,
new TestListener);
}
cout << "fn register: " << timer.GetTicks() << "\n";

timer.Start();
for (int x = 0; x < events; ++x) {
func_disp.Dispatch(lib::UniqueTypeID<TestEvent1>::kID, new TestEvent1(1, 2, 3));
}
cout << "fn dispatch: " << timer.GetTicks() << "\n\n";

// TEST SPECIFICEVENTDISPATCHER ----------------------------------------------
SpecificEventDispatcher<TestEvent1*> specific_disp;

timer.Start();
for (int x = 0; x < listeners; ++x) {
specific_disp.Register(&TestListener::OnTestEvent1, new TestListener);
}
cout << "sp register: " << timer.GetTicks() << "\n";

timer.Start();
for (int x = 0; x < events; ++x) {
specific_disp.Dispatch(new TestEvent1(1, 2, 3));
}
cout << "sp dispatch: " << timer.GetTicks() << "\n\n";

// TEST VARIADICEVENTDISPATCHER ----------------------------------------------
VariadicEventDispatcher variadic_disp;

timer.Start();
for (int x = 0; x < listeners; ++x) {
variadic_disp.Register(&TestListener::OnEvent, new TestListener);
}
cout << "va register: " << timer.GetTicks() << "\n";

timer.Start();
for (int x = 0; x < events; ++x) {
variadic_disp.Dispatch(new TestEvent1(1, 2, 3));
}
cout << "va dispatch: " << timer.GetTicks() << "\n\n";

// TEST INTERFACE-BASED EVENTDISPATCHER --------------------------------------
inp::EventDispatcher interface_disp;

timer.Start();
for (int x = 0; x < listeners; ++x) {
interface_disp.Register<TestEvent1>(new TestListener);
}
cout << "in register: " << timer.GetTicks() << "\n";

timer.Start();
for (int x = 0; x < events; ++x) {
interface_disp.Dispatch(new TestEvent1(1, 2, 3));
}
cout << "in dispatch: " << timer.GetTicks() << "\n";

cout << "\n";
system("pause");
return 0;
}


VariadicEventDispatcher:class VariadicEventDispatcher {
public:
// typedefs: ===============================================================
typedef std::vector<void*> ListenerVector;
typedef std::map<int, ListenerVector> ListenerMap;

// ctors/dtor: =============================================================
VariadicEventDispatcher() {}
virtual ~VariadicEventDispatcher() {}

// public functions: =======================================================
// - listeners_ access
template<typename... EventT>
void Register(void (*func)(EventT...));

template<typename... EventT, typename ClassT>
void Register(void (ClassT::*func)(EventT...), ClassT* obj);

// - dispatch event
template<typename... EventT>
void Dispatch(EventT... args);

private:
// vars: ===================================================================
ListenerMap listeners_;
};

// public functions ============================================================
// - listeners access ----------------------------------------------------------
template<typename... EventT>
void VariadicEventDispatcher::Register(void (*func)(EventT...)) {
listeners_[lib::UniqueTypeID<EventT...>::kID].push_back(
new std::function<void(EventT...)>(func));
}

template<typename... EventT, typename ClassT>
void VariadicEventDispatcher::Register(void (ClassT::*func)(EventT...),
ClassT* obj) {
listeners_[lib::UniqueTypeID<EventT...>::kID].push_back(
new std::function<void(EventT...)>(
std::bind(func, obj, std::placeholders::_1)));
}

// - dispatch event ------------------------------------------------------------
template<typename... EventT>
void VariadicEventDispatcher::Dispatch(EventT... event) {
//if (event == NULL) { return; } // disallow a NULL event

ListenerMap::iterator itr = listeners_.find(lib::UniqueTypeID<EventT...>::kID);
if (itr == listeners_.end()) { return; } // type not found

// dispatch event to the listeners
for (unsigned int x = 0; x < itr->second.size(); ++x) {
// I know, I know, this is fugly, but it didn't appear to affect
// performance to static_cast the void* to std::function* in my tests.
(*static_cast<std::function<void(EventT...)>*>(itr->second.at(x)))(event...);
}
}


SpecificEventDispatcher:template<typename EventT>
class SpecificEventDispatcher {
public:
// typedefs: ===============================================================
typedef std::function<void(EventT)> ListenerCallback;
typedef std::vector<ListenerCallback> ListenerVector;

// ctors/dtor: =============================================================
SpecificEventDispatcher() {}
virtual ~SpecificEventDispatcher() {}

// public functions: =======================================================
// - listeners_ access
void Register(void (*func)(EventT));

template<typename ClassT>
void Register(void (ClassT::*func)(EventT), ClassT* obj);

// - dispatch event
void Dispatch(EventT event);

private:
// vars: ===================================================================
ListenerVector listeners_;
};

template<typename EventT>
void SpecificEventDispatcher<EventT>::Register(void (*func)(EventT)) {
listeners_.push_back(func);
}

template<typename EventT> template<typename ClassT>
void SpecificEventDispatcher<EventT>::Register(void (ClassT::*func)(EventT),
ClassT* obj) {
listeners_.push_back(std::bind(func, obj, std::placeholders::_1));
}

// - dispatch event
template<typename EventT>
void SpecificEventDispatcher<EventT>::Dispatch(EventT event) {
if (event == NULL) { return; } // disallow a NULL event

// dispatch event to the listeners
for (unsigned int x = 0; x < listeners_.size(); ++x) {
listeners_.at(x)(event);
}
}


EventDispatcher (function based):class EventDispatcher {
public:
// typedefs: ===============================================================
typedef std::function<void(inp::EventBase*)> ListenerCallback;
typedef std::vector<ListenerCallback> ListenerVector;
typedef std::map<int, ListenerVector> ListenerMap;

// ctors/dtor: =============================================================
EventDispatcher() {}
virtual ~EventDispatcher() {}

// public functions: =======================================================
// - listeners_ access
void Register(int type, void (*func)(inp::EventBase*));

template<typename ClassT>
void Register(int type, void (ClassT::*func)(inp::EventBase*), ClassT* obj);

// - dispatch event
void Dispatch(int type, inp::EventBase* event);

private:
// vars: ===================================================================
ListenerMap listeners_;
};

// public functions ============================================================
template<typename ClassT>
void EventDispatcher::Register(int type, void (ClassT::*func)(inp::EventBase*), ClassT* obj) {
listeners_[type].push_back(std::bind(func, obj, std::placeholders::_1));
}

// .cpp file


// public functions ============================================================
// - listeners access ----------------------------------------------------------
void EventDispatcher::Register(int type, void (*func)(inp::EventBase*)) {
listeners_[type].push_back(func);
}

// - dispatch event ------------------------------------------------------------
void EventDispatcher::Dispatch(int type, inp::EventBase* event) {
if (event == NULL) { return; } // disallow a NULL event

ListenerMap::iterator itr = listeners_.find(type);
if (itr == listeners_.end()) { return; } // type not found

// dispatch event to the listeners
for (unsigned int x = 0; x < itr->second.size(); ++x) {
itr->second.at(x)(event);
}
}


inp::EventDispatcher (interface based):class EventDispatcher {
public:
// typedefs: ===============================================================
typedef std::vector<EventListenerBase_Interface*> ListenerVector;
typedef std::map<int, ListenerVector> ListenerMap;

// ctors/dtor: =============================================================
EventDispatcher() {}
virtual ~EventDispatcher() {}

// public functions: =======================================================
// - listeners_ access
template<typename EventT>
void Register(EventListener_Interface<EventT>* listener);

template<typename EventT>
void Unregister(EventListener_Interface<EventT>* listener);

template<typename EventT>
bool IsRegistered(const EventListener_Interface<EventT>& listener);

// - dispatch event
template<typename EventT>
void Dispatch(EventT* event);

template<typename EventT>
void DispatchAndDelete(EventT* event);

private:
// vars: ===================================================================
ListenerMap listeners_;
};

// public functions: ===========================================================
// - listeners_ access ---------------------------------------------------------
template<typename EventT>
void EventDispatcher::Register(EventListener_Interface<EventT>* listener) {
if (listener == NULL) { return; } // disallow a NULL listener
listeners_[lib::UniqueTypeID<EventT>::kID].push_back(listener);
}

template<typename EventT>
void EventDispatcher::Unregister(EventListener_Interface<EventT>* listener) {
if (listener == NULL) { return; } // disallow a NULL listener

ListenerMap::iterator itr = listeners_.find(lib::UniqueTypeID<EventT>::kID);

if (itr != listeners_.end()) { // event type found
// find the location in the vector
for (unsigned int x = 0; x < itr->second.size(); ++x) {
if (itr->second.at(x) == listener) {
if (itr->second.size() == 1) { // vector will be empty, so remove it
listeners_.erase(itr);

} else { // vector won't be empty, so just remove the listener
itr->second.erase(itr->second.begin() + x);
}
return;
}
}
}
}

template<typename EventT>
bool EventDispatcher::IsRegistered(
const EventListener_Interface<EventT>& listener) {
ListenerMap::iterator itr = listeners_.find(lib::UniqueTypeID<EventT>::kID);

if (itr != listeners_.end()) { // event type was found
for (unsigned int x = 0; x < itr->second.size(); ++x) {
if (itr->second.at(x) == &listener) { return true; } // listener was found
}
}

return false; // event type or listener was not found
}

// - dispatch event ------------------------------------------------------------
template<typename EventT>
void EventDispatcher::Dispatch(EventT* event) {
if (event == NULL) { return; } // disallow a NULL event

ListenerMap::iterator itr = listeners_.find(lib::UniqueTypeID<EventT>::kID);

if (itr == listeners_.end()) { return; } // event type not found

// dispatch event to the listeners
for (unsigned int x = 0; x < itr->second.size(); ++x) {
static_cast<EventListener_Interface<EventT>*>(
itr->second.at(x))->OnEvent(static_cast<EventT*>(event));
}
}

template<typename EventT>
void EventDispatcher::DispatchAndDelete(EventT* event) {
Dispatch(event);
delete event;
}

This topic is closed to new replies.

Advertisement