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

perf: Reduce the amount of redundant 'Change' data serialized by SyncDictionary #3610

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

cshunsinger
Copy link
Contributor

New logic is added here to track the changes that get made to a SyncDictionary which drastically reduces the amount of redundant data being sent over the network.

Now if several modifications get made to the same key, whether it is add/set/remove/clear, there will not be multiple "Change" entries recorded.

For example, if a dictionary has 10 different keys, and if each of the 10 keys have their values added/removed/set 1000 times each in a single frame, then there will still only be 10 "Change" elements.

A "Clear" operation will also clear all tracked "Changes". So in the above example where there are 10 changes, performing a OP_CLEAR operation would also destroy those 10 "Changes" and replace them with a single "Change" representing the OP_CLEAR.

The only exception to the above is this: OnSerializeAll, which will ensure that all existing changes will still be transmitted during the next OnSerializeDelta along with at-most 1 additional change for every distinct dictionary slot that gets modified at least once. This is the one situation where there might be a limited quantity of redundant "Changes" that get transmitted, but this is still limited.

Finally, while the default behavior of SyncDictionary is modified by these changes to prevent sending redundant change details, a boolean flag can be optionally passed to the SyncDictionary constructor. If set to true, the SyncDictionary will use the "old" method of tracking every single change regardless of whether or not the changes are redundant.

Please don't roast me too hard. I was just looking into some of Mirror's code and saw a TODO and thought I'd get some practice.

@cshunsinger cshunsinger changed the title Reduce the amount of redundant 'Change' data serialized by SyncDictionary perf: Reduce the amount of redundant 'Change' data serialized by SyncDictionary Sep 17, 2023
Copy link
Collaborator

@MrGadget1024 MrGadget1024 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make this backward compatible and non-breaking, syncAllChanges must default to true as an optional param in the constructor.

@cshunsinger
Copy link
Contributor Author

cshunsinger commented Sep 19, 2023

Makes sense. I'll tweak it after work. I'm gonna rename and refactor the boolean in that case so that false means "send all changes" because I don't like bools being true by default in an optional param.

@cshunsinger
Copy link
Contributor Author

The default behavior is now updated so that SyncDictionary will work as it always has. Setting the dictionary flag to true in the constructor will enable it to only send a limited quantity of changes per field.

@imerr
Copy link
Contributor

imerr commented Feb 1, 2024

Looking pretty good to me, any chance you can add tests?
As a start replacing

[TestFixture]
public class SyncDictionaryTest
{

with

    [TestFixture(false)]
    [TestFixture(true)]
    public class SyncDictionaryTest
    {
        private bool optimized;
        public SyncDictionaryTest(bool optimized)
        {
            this.optimized = optimized;
        }

and creating the syncdict's with the optimized flags in Setup

        [SetUp]
        public void SetUp()
        {
            serverSyncDictionary = new SyncDictionary<int, string>(optimized);
            clientSyncDictionary = new SyncDictionary<int, string>(optimized);

would run tests in both modes (which passes currently)

@MrGadget1024
Copy link
Collaborator

@cshunsinger If unit tests are beyond your skill level, just say so and we'll work them up for you.
Sorry this has been delayed, but we're finally able to look at outstanding PR's for merging.

@cshunsinger
Copy link
Contributor Author

I'll add the unit tests.
Sorry for not responding sooner, just very busy at the moment with my day job and a toddler.

@MrGadget1024 MrGadget1024 requested a review from imerr March 15, 2024 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Review enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants