Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace Explicit "Action" based event system with C#-style Task system #583

Closed
wants to merge 4 commits into from
Closed

Replace Explicit "Action" based event system with C#-style Task system #583

wants to merge 4 commits into from

Conversation

ddevec
Copy link
Contributor

@ddevec ddevec commented Dec 29, 2017

This is a major overhaul to the event system used by our code (sorry its so large). Some of the highlights:

  • No longer use Actions/ActionChains, events are now managed with "coroutines", enabled through C#'s TPL Task class, and a custom TaskScheduler.
  • Database loads no longer block the main thread (this was trivial to do with the new task structure).
  • TODO: Manage Tasks based on loaded/unloaded entities (e.g. stop player-bound tasks automatically when the player is unloaded, or corpse-decay tasks when a landblock is unloaded).

This is a huge change, that changes most of the files in the code... I couldn't really help it. I would recommend trying to check it out, and see how its working. IMO, its much cleaner, more efficient code than our prior system with more functionality.

I'm still debugging some of our fringe use-cases, so some bug fixes may be forthcoming, but figured I would get this up to let people review.

@spazmodica
Copy link
Contributor

Some items of note:

  1. Memory usage increase
    Current Master - 229 MB initial. After 2 hours stays steady around 229-230
    PR 583 - 269 MB initial. After 2 hours up to 289 MB
    Both versions the client logged in, teleported 10 times, then ran about 10 land blocks
  2. BlockingCollection use
    Since the CompleteAdding() is never called, what was the reasoning for "while (!runningTasks.IsAddingCompleted"? Place holder?
  3. Pulling and readding tasks.
    Have a server thread run the BlockingCollection then it can run the child tasks. Doing the pull and putting the same task back into the collection seems a strange approach.
  4. Use the BlockingCollection enumerator - GetConsumingEnumerable(). It will get the next item in the collection and can be called in a Parallel.Foreach statement
  5. Having everything run on its own thread does make a bit difficult. What about one thread per player, one for db, one for server, one for landblocks, etc. It may make the overhead to manage a bit more to code, but could make debugging easier.
  6. Future change to have a server shutdown command send the CompleteAdding() to the worldqueue.
  7. Did see bubbling up of exceptions. Both where nullable being referenced for a value.

@spazmodica
Copy link
Contributor

Like to call out the change to dropping "Handle" from many methods. clap Makes looking at the dropdown in VS much easier to find a method vs scrolling through so many Handle...

@ddevec
Copy link
Contributor Author

ddevec commented Jan 1, 2018

I haven't looked at the memory usage closely, I suspect the leak comes from lack of object cleaning (I'll look into it, although if you have good C# memory profiling tools, shoot me a message on discord, I've yet to have to debug memory leaks in C#).

The polling thread your referencing (with the BlockingCollection) is actually intended only for debugging purposes. If you don't have a thread waiting on a Task, the VS debugger will just eat the exception, making debugging challenging. This way the VS debugger reports the exception happens.

(2) I didn't kill the polling thread, because allowing the OS to clean the thread at server-shutdown should be fine, its purely for debugging. (4) I didn't use parallel waiting because its just a debugging tool and I don't care about performance. (3) Tasks are polled then re-added because I'm just waiting to get the exceptions, I don't actually care about the death of the task (and some tasks will never die so I don't truly want to block on them). -- I should probably only enable that code block in a "debug build".

(5) Its important to note, everything isn't run on "its own thread", there is only one thread running in the system, the Tasks are all using a custom scheduler, forcing non-preemptive unicore (cooperative) multitasking (more specifically, it guarantees the region-serializable [RS] memory-model around "await" statements). Technically we do only have 3 threads in this model (network, db, and core), but it appears that the logic for each action is working in its own thread (for ease of programming), just with an RS memory model.

I was also happy to get rid of the "HandleAction" functions -- they were a remanent of special serialization functions from when we were using multiple core-logic threads, which wound up being overly complicated for unneeded performance gain. Its good to have that older system cleaned out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants