-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Improve sync performance #10657
Improve sync performance #10657
Conversation
Caching the result of the function lookup allows the actor to calculate all the sync hashes for its syncable traits faster as it does not need to repeat the lookup each time.
@@ -62,15 +67,13 @@ static void EmitSyncOpcodes(Type type, ILGenerator il) | |||
il.Emit(OpCodes.Ldc_I4, 0x555); | |||
il.MarkLabel(l); | |||
} | |||
else if (type.HasAttribute<SyncAttribute>()) |
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.
If we don't use this, could you also remove it from all other places?
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.
The trait lookup isn't gone, just cached at https://github.com/OpenRA/OpenRA/pull/10657/files#diff-25e3f19d154f93bcb747e9f12c252028R126.
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.
Nevermind, I also got confused, but SyncAttribute
is required for lint rules and used in SyncReport
.
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.
These particular lines are dead code - we only apply the sync attribute to fields and properties (this is enforced by the attribute usage). In this context type
will be either a class or struct definition. Therefore, this condition is always false.
I'm not removing the sync attribute or changing how it works - I'm just removing this useless code.
👍 / ✅ otherwise. Looks fine ingame and the game desyncs correctly. |
Jackpot! |
Previously, generating the sync hash for each syncable trait required a lookup to find the hash function every tick, as well as lookups for the syncable traits.
Instead, we now cache these lookups on actor creation as they do not change. This means we avoid repeated lookups for the traits and the hash functions.
On the RA shellmap, this reduces time spent syncing the world state from 9.65% of total CPU to 6.88%.