-
Notifications
You must be signed in to change notification settings - Fork 1k
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
experimenting with RemoteActorRefProvider address resolution performance #5228
experimenting with RemoteActorRefProvider address resolution performance #5228
Conversation
STDDEV is way up here - going to re-run it. |
@to11mtm should we just fix the |
That would probably be the better thing to do TBH. |
And maybe there is a fast check to return early?
|
The foreach is not good too. Because the Transport.Addresses is of type ISet the simple and obviousContains method is the best fit
see: https://sharplab.io/#gist:b0a2cc23e7748f71577070b2748e462e |
Great suggestions @Zetanova - I'll update the PR with some numbers for those later this week. |
Final numbers look good to me! |
src/core/Akka/Actor/Address.cs
Outdated
@@ -256,7 +258,8 @@ public Address WithPort(int? port = null) | |||
/// <returns><c>true</c> if both addresses are not equal; otherwise <c>false</c></returns> | |||
public static bool operator !=(Address left, Address right) | |||
{ | |||
return !Equals(left, right); | |||
return !ReferenceEquals(left, right) && | |||
(left?.Equals(right) ?? true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks incorrect. if left
equals right
we should return false. (I think the null coalesce is making it hard to infer here)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah based on the test suite I probably did that wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I thought I was !-ing the entire statement, not just the first clause.
The equals itself is not optimal, the Port nullable can be better compared. The See code at: |
public bool Equals(Address other)
{
return other != null
&& string.Equals(_host, other._host)
&& string.Equals(_system, other._system)
&& string.Equals(_protocol, other._protocol)
&& (_port ?? 0) == (other._port ?? 0);
} You want the port comparison to go first - as it's the fastest route to exit the loop for two values who are not equal. You also still want to check for equality by reference in this method in case it's called directly via the |
Final numbers:
Looks to me like the technical errors have been resolved as well. |
I'm not sure... the ASM generated looks too sparse for what we're doing here. I'm not sure if Sharplab is acccurate here. I would expect something like this would be the 'best' way to do ==:
This is based on how |
Thanks @to11mtm - I'll make that change and then wrap this PR up. |
@Zetanova you've been a great help on this PR btw - really appreciate your great feedback. |
…/github.com/Aaronontheweb/akka.net into perf/RemoteActorRefProvider-Address-resolve
I looked at the whole class The public bool HasAddress(Address address)
{
retrun address.Equals(_local.RootPath.Address) || Transport.Addresses.Contains(address);
} then the cache extension instances do not need to be volatile, private ActorRefResolveThreadLocalCache _actorRefResolveThreadLocalCache;
private ActorPathThreadLocalCache _actorPathThreadLocalCache;
public virtual void Init(ActorSystemImpl system)
{
_system = system;
//moved up
_actorRefResolveThreadLocalCache = ActorRefResolveThreadLocalCache.For(system);
_actorPathThreadLocalCache = ActorPathThreadLocalCache.For(system);
_local.Init(system);
//rest of code
} Then I made an draft at #5273 |
Not a huge deal from a perf point of view - |
@Aaronontheweb Why is there a AddressCache that caches the Address with the key as ActorPathString and an ActorPathCache that makes the same? |
@Aaronontheweb I made some improvements, volatile should/can be removed from Then I found a way to remove AddressCache and use the same ActorPathCache as RemoteActorRefProvider |
Per @to11mtm 's reporting in our Gitter chat room...
RemotePingPong
Before
After
Sharding Performance
Before
After