fix(NetworkBehaviour): removing NB that belong to another NI from list

[James-Frowen]

If there are Nested NetworkBehaviours and NetworkIdentity then there can be a problem where the parent NetworkIdentity will find Behaviour that belong to another child NetworkIdentity.

This will remove those NB from the current NI's array and then resize the array to be the correct size

fixes: #973

[James-Frowen]

We should test the performance of this vs LINQ version, because the linq version will be a lot simpler

[sonarcloud]

SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

52.2% Coverage
0.0% Duplication

[github-actions]

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days.

[JGroxz]

I'll make a test to check the speed of this code vs the LINQ version proposed in #1006 (in this comment).

P.S. Please let me know if such things need a new issue opened or if something else has to be done differently, I'm fairly new here.

[JGroxz]

@James-Frowen So I made a prefab with NI, which has 100 nested NIs inside it. Each NI, included the root one, has 100 NetworkTransforms under it. Which totals at 100100 NBs.

Added LINQ code from #1006 and Array code from #970 to NI, and an enum field to set the test mode. Test script:

1. Spawns prefab with nested NIs.
2. Sets test mode on the root NI.
3. Initializes root NI using _ = rootIdentity.NetworkBehaviours;
4. Initialization speed is logged into console (timed with System.Diagnostics.Stopwatch).

Destroy the object, repeat with Array. I also added an alternative method using List.

Results from my machine (Intel i7-9750H):

Note: the results fluctuate a bit between runs and sometimes LINQ takes ~1-2 ms longer, becoming slower than Array. Given that the difference is marginal, I suggest we use the LINQ version, because its readability in much better.

@James-Frowen I can push the modified NI to the branch of #1006 and provide unitypackage with the prefab, so you can test it. All test code is in a separate #region, so it will be easy to remove it after.

[James-Frowen]

we need to be careful with GC/alloc here too because that could cause performance problem too.

Ideally we have the default case (where there is no child NI) to not allocate so it wont effect anything in existing projects. But I'm not sure if there is a way to have it not allocate when there is child NI.

I might be overthinking all this tho, and maybe it wont matter either way

[JGroxz]

we need to be careful with GC/alloc here too because that could cause performance problem too.

From the top of my head, with the LINQ solution there are 2 places where allocations happen:

1. GetComponentsInChildren() when we look for child NBs. This can be mitigated by having a pre-allocated static List<NetworkBehaviour> which will serve as a cache that we pass to GetComponentsInChildren() (same as I do in code for Unity 2019 in fix(NetworkBehaviour): make NetworkBehaviour always look for NetworkIdentity in parents #1006). This will avoid allocations if the number of child NBs does not exceed the original array's size. But what should the default size be in this case?
2. LINQ call, the result of which gets assigned to networkBehaviours cache array. However, it gets allocated anyway when NetworkBehaviours property is initialized, even with no child NIs. So maybe not a big deal for now?

[James-Frowen]

Static list passed into GetComponentsInChildren sounds promising. Doing .toArray() after should be fine because we are keeping the array anyway.

I think .Where itself allocates so we'll have to check that vs a loop calling remove on the list

[JGroxz]

.Where does indeed allocate. I have combined your logic with a cached List and tested the performance, it is now as fast as LINQ implementation, but without extra allocation that came from .Where.

The changed parts look something like this:

// cache list to call GetComponentsInChildren() with no allocations
private static List<NetworkBehaviour> ChildNetworkBehavioursCache = new List<NetworkBehaviour>(1024);

private NetworkBehaviour[] FilterChildNetworkBehaviours()
{
    // place NBs linked to this NI to the beginning of the list and keep track of index/count
    int linkedNetworkBehaviourIndex = 0;
    for (int r = 0; r < ChildNetworkBehavioursCache.Count; r++)
    {
        var item = ChildNetworkBehavioursCache[r];
        if (item.Identity != this) continue;

        ChildNetworkBehavioursCache[linkedNetworkBehaviourIndex] = item;
        linkedNetworkBehaviourIndex++;
    }

    // copy part of the list containing linked NBs to a new array
    int linkedNetworkBehavioursCount = linkedNetworkBehaviourIndex;
    Debug.Log(linkedNetworkBehavioursCount);
    var components = new NetworkBehaviour[linkedNetworkBehavioursCount];
    ChildNetworkBehavioursCache.CopyTo(0, components, 0, linkedNetworkBehavioursCount);

    return components;
}

I have already pushed it to the new branch in my fork, so I will create a new PR tomorrow and reference this PR in there.

[James-Frowen]

I've managed to benchmark these 2 options

[BenchmarkMethod(name: "List removeAt", description: "2: no gc")]
private NetworkBehaviour[] FindBehaviourForThisIdentity2()
{
    GetComponentsInChildren<NetworkBehaviour>(true, childNetworkBehavioursCache);

    // start at last so we can remove from end of array instead of start
    for (int i = childNetworkBehavioursCache.Count - 1; i >= 0; i--)
    {
        NetworkBehaviour item = childNetworkBehavioursCache[i];
        if (item.Identity != this)
        {
            childNetworkBehavioursCache.RemoveAt(i);
        }
    }

    return childNetworkBehavioursCache.ToArray();
}

[BenchmarkMethod(name: "List move copy", description: "4: no gc")]
private NetworkBehaviour[] FindBehaviourForThisIdentity4()
{
    GetComponentsInChildren<NetworkBehaviour>(true, childNetworkBehavioursCache);

    // keep track of read and write index
    // if we find a NB that belongs to another NI then we set it to null
    // then if any have been set to null we shift the remaining NB down to the write index as we continue the loop
    // after the loop we resize the array if we have removed any
    int write = 0;
    for (int read = 0; read < childNetworkBehavioursCache.Count; read++)
    {
        NetworkBehaviour item = childNetworkBehavioursCache[read];
        // if not this identity then skip
        if (item.Identity != this)
            continue;

        // write item to start of array, this is so that if one is skipped all kept ones are on low end of array
        childNetworkBehavioursCache[write] = item;
        write++;
    }

    var temp = new NetworkBehaviour[write];
    childNetworkBehavioursCache.CopyTo(0, temp, 0, write);
    return temp;
}

Results

Name	Description	Category	Mean	Ratio	StdDev	StdError	min	max
List move copy	4: no gc		2.033 us		2.316 us	0.073 us	1.435 us	18.080 us
List removeAt	2: no gc		2.097 us		2.578 us	0.082 us	1.400 us	19.895 us

they seems to be mostly the same for performance (probably because the StdDev is so high)

I'm probably going to go with the RemoveAt one because it looks simpler

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

85.7% Coverage
0.0% Duplication

[github-actions]

🎉 This PR is included in version 113.3.2 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀