Spodii / netgore

Cross platform online rpg engine using C# and SFML
http://www.netgore.com/
40 stars 16 forks source link

Using EventHandler instead of custom event handler delegates #279

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
I've used custom delegates for event handlers in pretty much every event. 
Lately I've been doing it more just out of habit and consistency with the rest 
of the code. Though it adds a lot of needless clutter, and using EventHandler 
is the more standard (and recommended) approach.

Originally, I decided to not use EventHandler since (1) I didn't like passing 
the sender as an object, and (2) I didn't like EventArgs. Reasons:

1. Passing the sender as a specific type is no problem in NetGore, but it can 
be a little annoying for those trying to forward and/or collect events 
(something which I've never really had to do in NetGore). On the other hand, 
up-casting from object is done frequently, its troublesome to do, and it 
removes the ability to know at compile/design time what type of object the 
sender will be.

2. EventArgs looks tacky when passing an empty argument all the time, and 
having to create an object instance just to pass a variable or two seems like a 
lot of overhead. The first part can be resolved with a custom EventHandler with 
no EventArgs, but the latter cannot be fixed (unless a single mutable EventArgs 
instance were to be used, but that'd be far too dangerous).

Through benchmarking a test app, I found that:
 - Base line: A simple event invocation can be made 150k-200k times per millisecond
 - Invoking an event with no EventArgs (just sender) is about 30% faster
 - Creating an EventArgs instance is about 50% slower than EventArgs.Empty (which may be far more expensive when there is far more garbage and GC is more expensive)
 - Passing a struct directly is about 50% faster than putting it in a new EventArgs instance (showing that an empty EventArgs call is nearly free and all the overhead is from object creation/collection... as expected)

In reality, the overhead from events is absolutely tiny - maybe a total of a 
second or two each day the server is running. But I'd also like to make more 
use of events, so want to get a good foundation, and its the garbage generation 
that worries me more than anything.

I'll have to research things a bit more, but what I think I am going to do is:
1. Introduce an EventHandler that takes no EventArgs (to clearly show that the 
event really has no arguments)
2. Introduce an EventHandler that allows you to specify the type for sender (I 
really, really hate up-casting)
3. Create an EventArgs that takes a generic argument (basically a tuple; for 
the sake of avoiding creating a ton of classes holding just a single struct or 
two)

#1 breaks the design concept of being able to rely on all events being able to 
support the signature (object, EventArgs), but I just find having an EventArgs 
that is never used to be even more confusing and troublesome than just not 
including it.

Related to this, it'd be nice to also use an extension method to safely call 
events instead of the approach used now. By safely call events, I mean 1) check 
that the event is not null and 2) grab the event reference to a local variable 
first to avoid incredibly rare race conditions. Right now, most all of NetGore 
only does #1.

Original issue reported on code.google.com by Spodiii on 3 Dec 2010 at 7:15

GoogleCodeExporter commented 9 years ago
Also, in all cases where I have the virtual OnEvent method, the event handler 
should be invoked in this method (doesn't matter exactly where in the body). 
This is the standard used by the .NET guidelines. I originally avoided this to 
specifically prevent overriding OnEvent and not calling the base class 
resulting in the event still being invoked, but apparently that isn't the 
approach used. Which makes sense since it provides more power to derived 
classes, giving them the ability to mutate the event arguments (such as the 
common Cancel event argument) before it is ever raised, effectively allowing 
the derived class to fully handle and break execution of events.

Original comment by Spodiii on 3 Dec 2010 at 7:30

GoogleCodeExporter commented 9 years ago
When done converting, the following should go back to using custom event 
handlers since they fire frequently enough (especially in the server) to 
justify the micro-optimizing:
 - ISpatial.Moved
 - Possibly other ISpatial stuff
 - Is there anything related to packet I/O being triggered?

Also, using only the .Raise() extension, I'm not going to be able to avoid 
creating the EventArgs if there are no listeners, which sucks. Don't think 
there is any good way around it, either. So I might have to continue to use the 
if check before-hand just to avoid that needless (creating the EventArgs for 
each event raise isn't so bad, but for each time an event CAN raise, that is a 
lot...).

Original comment by Spodiii on 4 Dec 2010 at 7:20

GoogleCodeExporter commented 9 years ago

Original comment by Spodiii on 6 Dec 2010 at 6:46