Improve performance of Equals function in IndexPath #269
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…two IndexPaths in the Equals function
* _hashCode is an int, not a string * CalculateHashCode is not overriding any method * We can't use 'this' object before all of its fields are assigned, so CalculateHashCode must be static
Some tests were not passing because of two issues: * The _hashCode value of the `default` IndexPath was 0, because it was not calculated in the constructor. This is wrong, it must have the same value as an empty IndexPath. To fix it, access the _hashCode by the GetHashCode method, and if it's 0, recalculate it. * The hash code of an IndexPath with just an index was not the same as the one of an IndexPath with a path containing the same index value. We must subtract one to the index for them to be equal
danipen
reviewed
Mar 8, 2024
There was a problem hiding this comment.
@alvaro-berruezo-unity3d, I just have a question about your changes.
| @@ -145,17 +155,22 @@ static IEnumerator<int> EnumerateSingleOrEmpty(int index) | |||
| } | |||
|
|
|||
| public override int GetHashCode() | |||
| { | |||
| return _hashCode == 0 ? CalculateHashCode(_path, _index) : _hashCode; | |||
There was a problem hiding this comment.
Why do we need this check here?
return _hashCode == 0 ? CalculateHashCode(_path, _index) : _hashCode;
I would expect to "just" return _hashCode since it's calculated in the constructor.
There was a problem hiding this comment.
This is for when the IndexPath is created as the default object (e.g. row.ModelIndex == default). In this case the hash code should be the same as the one in an empty IndexPath, but it's 0 because the constructor is not called
|
The performance issue is already fixed by #273 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does the pull request do?
Improves the performance of the
Equalsfunction, as well as the==and!=operations, by pre-calculating the hash code in the constructor and using it to compare the objects.What is the current behavior?
When two
IndexPathobjects are compared for equality, theCompareTomethod is executed, which can be too slow when comparing a large number ofIndexPathsThe
IndexPathscomparison is a limitation when trying to expand all nodes in a large tree, because it is a very heavily used operation used to convert model indexes to row indexes.What is the updated/expected behavior with this PR?
In every constructor of the
IndexPathwe calculate the hash code and store it in a member. Then this member is used whenever we need to compare twoIndexPathobjects for equality, which is much faster than using theCompareTomethod.