ReceiveActor - Removing MatchBuilder and making use of handlers. #7498
Conversation
| // Moved out and better tested | ||
| class HandlerSet | ||
| { | ||
| public HandlerSet() |
There was a problem hiding this comment.
The idea here is having something that handles the calls for each of the states of the actor.
- TypedHandlers - Handlers when we have the type information
- HandleObject - Handles calls when the type information supplied is object
- Handle Any - The catch all handler
Now that I am reviewing this I feel like the naming should be a bit more consistent
| } | ||
|
|
||
| // Attempting to make more use of generics to avoid boxing | ||
| class TypeHandler<T> : ITypeHandler |
There was a problem hiding this comment.
Have an interface that impliments some methods that can be used during the playback of messages. This was seemingly the nicest way to be able to handle generics and handle object calls.
There was a problem hiding this comment.
Pattern makes sense to me.
That said, I wonder if A more aggressive 'TryHandle(object message)` is warranted (i.e. to rely less on PGO/etc to avoid multiple unboxes) but benchmarks can be used to check benefit if any.
| handlers.TypedHandlers[typeof(T)] = handlerList; | ||
| } | ||
|
|
||
| var typeHandler = new TypeHandler<T> |
There was a problem hiding this comment.
Handlers where the Generic types are present and can be used
|
|
||
| if (typeof(T) == typeof(object)) | ||
| { | ||
| handlers.HandleObject.Add(new TypeHandler<object> |
There was a problem hiding this comment.
If the type supplied is object needed to make use of an object type handler. This is one of the things that I was not a big fan of but could not think of a more elegant way of doing it.
| } | ||
|
|
||
| // Have to use object here as dont have the generic type information | ||
| var typeHandler = new TypeHandler<object> |
There was a problem hiding this comment.
Because the generic information is not present here we needed to resort to using the object type.
|
@mhbuck I'm getting 351 test failures here so there might be an issue with the way we're doing message handling under this PR |
|
Will expand running the tests locally and narrow it down. 👍 |
I am pretty sure it is down to handling of concrete types vs interfaces etc. I took a pretty naive approach for handling. I will have a look into the current logic and work out a better approach. |
There was a problem hiding this comment.
Left some initial feedback, need to pull it down and look further though.
Impressive at first look!
Edited to add:
Didn't look at benchmark numbers first, Now that I have that gives me some things to dig deeper into. I think some of the feedback may help, It may also be worth looking at profiling to see where hotspots are and how those can be mitigated.
| HandleObject = new List<ITypeHandler>(); | ||
| } | ||
|
|
||
| public Dictionary<Type, List<ITypeHandler>> TypedHandlers { get; set; } |
There was a problem hiding this comment.
Where this can get tricky, is that I am not sure whether the overall implementation has the same 'inheritance/execution rules' as existing MatchBuilder.
A fun test case would be
public interface IFoo
{
}
public class FooA : IFoo
{
}
public class FooB : IFoo
{
}
///
Receive<IFoo>(/**/);
Receive<FooA>(/**/);
Receive<FooB>(/**/);
possibly also playing with predicate rules.
There was a problem hiding this comment.
Yeah this scenario is what is breaking most of the tests in clustering because I believe it makes use of this. I am working on building out the tests to cover the inheritance cases and get a better understanding the current matchbuilder functionality and where my approach is lacking. It will also make the receive actor tests more robust.
These tests are what I need to dig into more and understand better https://github.com/akkadotnet/akka.net/tree/dev/src/core/Akka.Tests/MatchHandler
| HandleObject = new List<ITypeHandler>(); | ||
| } | ||
|
|
||
| public Dictionary<Type, List<ITypeHandler>> TypedHandlers { get; set; } |
There was a problem hiding this comment.
But also, any reasons for not having it be a get-only? I get that HandleAny can mutate but not sure about this one (or HandleObject
| } | ||
|
|
||
| // Attempting to make more use of generics to avoid boxing | ||
| class TypeHandler<T> : ITypeHandler |
There was a problem hiding this comment.
Pattern makes sense to me.
That said, I wonder if A more aggressive 'TryHandle(object message)` is warranted (i.e. to rely less on PGO/etc to avoid multiple unboxes) but benchmarks can be used to check benefit if any.
| { | ||
| var wasHandled = partialAction(message); | ||
| if(!wasHandled && _shouldUnhandle) | ||
| var messageType = message.GetType(); |
There was a problem hiding this comment.
this should probably get inlined into the TryGetValue (or if we need a different lookup call).
| currentHandler.HandleAny(message); | ||
| } | ||
|
|
||
| if (_shouldUnhandle) |
There was a problem hiding this comment.
Might be a stupid question, but should this also get hit if HandleAny is null? (or maybe other logic concern?)
This is probably not the most ideal code. I want to make sure the tests are passing better before getting to cleaning it up
Fixes #
This is part of the changed needed to resolve #2811
Changes
Here the changes are to not make use of the match builder and just make use of the handlers. The approach I have taken is very similar in structure to what was existing but making use of handlers for the various types. There could be merit in removing more of the logic to simplify the class more.
This approach has some internal classes (I created as internal as an initial approach I can see merit in pulling these out to be more clean within the class.
While all of these tests do pass I do think there are some edge cases that this does not cover when reading the comments in the MatchBuilder class. If the general approach of the handlers is what is wanted I will go into the comments and edge cases in more detail.
Checklist
For significant changes, please ensure that the following have been completed (delete if not relevant):
Latest
devBenchmarksNote I pushed the dotnet up to 8.0 because I did not have 6.0 installed on this machine
This PR's Benchmarks
So as the numbers are showing the performance has gone backwards