Optimize index bounds check for immutable sorted set and list #53266
Conversation
- The bounds check to determine if a given index is >= 0 and < this.Count
is only necessary on the first call to ItemRef.
- The recursive steps within ItemRef do not need to continuously
do this bounds check on these immutable data structures.
- Proof:
Elimination of index >= 0 bounds check:
The first call to ItemRef checks if index >= 0.
If we recurse on the left node, the index value does not change.
If we recurse on the right node, index > _left._count. Then
index - _left._count - 1 >= 0.
Elimination of index < this.Count:
The first call to ItemRef checks if index < this.Count. Then
the given index must lie somewhere in this tree and
(**) index < this.Count == left.Count + right.Count + 1.
If we recurse on the left node, the index value does not change
and a check is already made to determine that index < _left.Count.
If we recurse on the right node, then we need to be sure that
index - _left.count - 1 < _right.Count. But this is just a
rearrangement of (**).
|
Tagging subscribers to this area: @eiriktsarpalis Issue Details
|
|
Thanks. Can you share benchmark results showing that the extra code required here to avoid that range check actually makes a meaningful difference? (Note that range check itself could be optimized slightly as well, to |
|
Sure @stephentoub dotnet/performance diff with/without this change using --filter "*ImmutableSortedSet*" "*ImmutableList*" summary: No Slower results for the provided threshold = 2% and noise filter = 25ns.
|
|
@stephentoub thanks, I did encounter some places with these |
Thanks for sharing numbers. I was surprised by the magnitude of the differences you're seeing, so I tried locally, and I'm not seeing results anything like that, e.g.
Can you share more about how you're getting your numbers? |
...s/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableSortedSet_1.Node.cs
Outdated
Show resolved
Hide resolved
...raries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList_1.Node.cs
Outdated
Show resolved
Hide resolved
|
The code changes look fine, but for the most part this doesn't appear to consistently move the needle, so I'm not sure it's worth the churn. |
|
Thanks @stephentoub , it looks like some recent commits have changed the perf numbers. Here's a breakdown of my investigation (Windows x64): without my change + base commit = b443b8c (May 24, 2021) (The numbers I provided earlier were based on this) Today I cloned dotnet/runtime and created the four coreruns above and reran the microbenchmarks: Here's an example of the commands I use: then I diff the results using dotnet/performance's resultsComparer: Results: without my change + base commit = b443b8c (May 24, 2021) summary:
without my change + base commit = d43e886 (May 28, 2021) summary:
Notes: Building with the new base = d43e886 (May 28, 2021) seems to have changed some of the perf numbers around. Some faster and some slower than before: without my change + base commit = b443b8c (May 24, 2021) summary:
So this change still shows some decent results with the latest base commit = d43e886 (May 28, 2021), but you're right not nearly as good as the numbers on base commit base commit = b443b8c (May 24, 2021). I'll follow your recommendation. |
Change from internal to private for unchecked methods. Co-authored-by: Stephen Toub <stoub@microsoft.com>
|
Latest .NET6 (commit ac87f00) before/after this change: summary:
|
The bounds check to determine if a given index is >= 0 and < this.Count
is only necessary on the first call to ItemRef.
The recursive steps within ItemRef do not need to continuously
do this bounds check on these immutable data structures.
Proof:
Elimination of index >= 0 bounds check:
The first call to ItemRef checks if index >= 0.
If we recurse on the left node, the index value does not change.
If we recurse on the right node, index > _left._count. Then
index - _left._count - 1 >= 0.
Elimination of index < this.Count:
The first call to ItemRef checks if index < this.Count. Then
the given index must lie somewhere in this tree and
(**) index < this.Count == left.Count + right.Count + 1.
If we recurse on the left node, the index value does not change
and a check is already made to determine that index < _left.Count.
If we recurse on the right node, then we need to be sure that
index - _left.count - 1 < _right.Count. But this is just a
rearrangement of (**).