Actor task scheduler refactoring

[JeffCyr]

This pull request is a refactoring to fix the issues discussed in issue #1481. The major change with the old design is that each actor now have its own (lazy loaded) ActorTaskScheduler. CallContext is no longer required to flow the state so it should achieve better performance.

[rogeralsing]

Looks good! 👍

[rogeralsing]

Can you squash the commits into one?

[JeffCyr]

@rogeralsing Done!

[rogeralsing]

Thanks! 👍

[Aaronontheweb]

I'm sorry, but this should be performance tested with a benchmark before it's merged.

CallContext is no longer required to flow the state so it should achieve better performance.

Probably, but I'll never take a person's word for it.

Reverting this until we have benchmark data to back this up.

[JeffCyr]

Sure I'll create a benchmark

[Aaronontheweb]

Thanks @JeffCyr !

[rogeralsing]

I do like the design used here.
There is a lot of headache that disappears by removing the CallContext.
As far as I can tell, @JeffCyr is correct, executing tasks and flowing async state w/o call context is faster.
That being said, there is a tradeoff by using more memory instead, as each async actor also holds a ref to its own TaskScheduler

This will allocate 8 bytes extra for all actors due to the task scheduler reference.
And an unknown amount of memory for actors that use the task scheduler.

I'd be more interested in seeing the memory footprint of async actors than performance in this case as there is nothing affecting the default message pipeline.

But all in all, I'm in favor for this design, it would probably solve the current Mono async problem we have also where the actor Context is null after async operations.

[JeffCyr]

I updated my branch to revert the revert and include a benchmark, can we reopen this PR or do I need to create another one?

I added an optional AsyncActor test in the PingPong benchmark PingPong --async.

Results shows that the refactored ActorTaskScheduler is 2-3x faster than the original.

Before:

After:

[JeffCyr]

@rogeralsing Regarding the memory footprint, there's not much we can do for the 8 extra bytes, but since ActorCell already have a lot of fields, it should not make a big difference proportionally.

For actors that are initializing their TaskScheduler, if you check http://referencesource.microsoft.com/#mscorlib/system/threading/Tasks/TaskScheduler.cs,b76a4a6f77962f28

You can see that TaskScheduler is very lightweight (it only have an int field), it's just sad that they add a weakref of every TaskScheduler in a static dictionary (custom hash table) which is only for debugging purpose. They should have used a debugging flag like Task.s_asyncDebuggingEnabled does. I will probably create an issue in coreclr about this but we won't see the fix anytime soon.

We could benchmark the memory pressure when millions of actors initialize their TaskScheduler, but I guess it would be that same as keeping a hash table referencing all actors, which would probably be acceptable.

[stefansedich]

Any reason for the m_ ?

[JeffCyr]

Old habits :) I'll fix that

[JeffCyr]

I created the issue 2189 about TaskScheduler on coreclr.

Here's the benchmark result for the creation of 10 millions TaskScheduler:

Real TaskScheduler test:
Memory usage: 420.48 MB
Total time: 00:00:13.9793632

Fake TaskScheduler test:
Memory usage: 148.89 MB
Total time: 00:00:01.0347952

The memory usage is not that bad and the cpu usage is bad but would probably be amortized by the application without notice.

There is a workaround by creating the ActorTaskScheduler with FormatterServices.GetUninitializedObject to bypass the constructor, but that would be a very nasty hack!

[Aaronontheweb]

@JeffCyr you'll need to open a new PR

[Horusiath]

@JeffCyr concerning your screens from the benchmark, did you noticed serious perf drop in standard actors after the changes?

[JeffCyr]

@Horusiath The code path for standard actor is not affected because the ActorTaskScheduler is only created if an actor uses it. My screenshots may have small variations because the payload on my machine was not the same when I did both runs, I ran the benchmark again and got similar results with and without the changes.

[rogeralsing]

The reason I brought up the 8 extra bytes earlier is that once the actor reaches a certain size, there will be effects on the CPU mem cache pipeline.
JVM Akka have worked hard to bring the size down to prevent this.

I think that the .NET actors are still too big to benefit from any low level optimizations, but there could be potential side effects when altering the size of a raw actor cell.
(I dont think there is in this case, but in theory)

[JeffCyr]

If this becomes an issue, we could group some infrequently accessed fields in another class and access them with a level of indirection.