Skip to content

chore: Improve performance of NetworkList set operation #3587

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

Merged
merged 5 commits into from
Aug 7, 2025

Conversation

EmandM
Copy link
Collaborator

@EmandM EmandM commented Aug 6, 2025

continues: #3585

This PR adds an equality check to the NetworkList<T> indexer setter, aligning its behavior with NetworkVariable<T>.Value. Since NetworkList<T> already constrains T to IEquatable<T>, this change is both safe and broadly applicable. It avoids redundant assignments when the new value is equal to the current value, which improves runtime efficiency and avoids unnecessary event dispatch and network synchronization.

Changelog

  • Changed: Optimized NetworkList<T> indexer setter to skip operations when the new value equals the existing value, improving performance by avoiding unnecessary list events and network synchronization.

Testing & QA

This is a very small optimization in an area of code that is deeply covered by unit and integration tests. No manual testing is required.

Documentation

As there are no API or behaviour changes, no documentation is required

Backport

This is a new optimization and so doesn't need to be backported

harayuu9 and others added 3 commits August 6, 2025 16:44
…nged

Added equality check in NetworkList indexer setter to avoid unnecessary operations when the new value equals the existing value. This improves performance by preventing redundant list events and network synchronization.
Copy link

@NikiWalker NikiWalker left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -617,6 +617,13 @@ public T this[int index]
}

var previousValue = m_List[index];

// Compare the Value being applied to the current value

Choose a reason for hiding this comment

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

nit: Comment why, not what. E.g. The CHANGELOG comment above would be good as a comment here instead, for example

Copy link

Choose a reason for hiding this comment

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

As context, this was copied from the NetworkVariable implementation. I agree the comment is somewhat self-evident and likely unnecessary.

// Compare the Value being applied to the current value
if (!NetworkVariableSerialization<T>.AreEqual(ref m_InternalValue, ref value))

Copy link
Collaborator

@NoelStephensUnity NoelStephensUnity left a comment

Choose a reason for hiding this comment

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

:godmode:

@EmandM EmandM enabled auto-merge (squash) August 7, 2025 17:53
@EmandM EmandM merged commit 9f7a7a8 into develop-2.0.0 Aug 7, 2025
25 of 27 checks passed
@EmandM EmandM deleted the chore/network-list-performance-set-operation branch August 7, 2025 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants