From fa983abb00c9758aac17110533df8d09caad6a59 Mon Sep 17 00:00:00 2001 From: Mansoor Saqib Date: Tue, 25 May 2021 19:19:51 -0700 Subject: [PATCH 1/4] Optimize index bounds check for immutable sorted set and list - 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 (**). --- .../Immutable/ImmutableList_1.Node.cs | 20 ++++++++++++++++-- .../Immutable/ImmutableSortedSet_1.Node.cs | 21 +++++++++++++++++-- 2 files changed, 37 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList_1.Node.cs b/src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList_1.Node.cs index ef0381384f29ac..7cde4fce30e8c6 100644 --- a/src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList_1.Node.cs +++ b/src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList_1.Node.cs @@ -199,12 +199,28 @@ internal ref readonly T ItemRef(int index) Debug.Assert(_left != null && _right != null); if (index < _left._count) { - return ref _left.ItemRef(index); + return ref _left.ItemRefUnchecked(index); } if (index > _left._count) { - return ref _right.ItemRef(index - _left._count - 1); + return ref _right.ItemRefUnchecked(index - _left._count - 1); + } + + return ref _key; + } + + internal ref readonly T ItemRefUnchecked(int index) + { + Debug.Assert(_left != null && _right != null); + if (index < _left._count) + { + return ref _left.ItemRefUnchecked(index); + } + + if (index > _left._count) + { + return ref _right.ItemRefUnchecked(index - _left._count - 1); } return ref _key; diff --git a/src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableSortedSet_1.Node.cs b/src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableSortedSet_1.Node.cs index 89b044c0ce9793..ebd3940e4c1da6 100644 --- a/src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableSortedSet_1.Node.cs +++ b/src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableSortedSet_1.Node.cs @@ -265,12 +265,29 @@ internal ref readonly T ItemRef(int index) if (index < _left._count) { - return ref _left.ItemRef(index); + return ref _left.ItemRefUnchecked(index); } if (index > _left._count) { - return ref _right.ItemRef(index - _left._count - 1); + return ref _right.ItemRefUnchecked(index - _left._count - 1); + } + + return ref _key; + } + + internal ref readonly T ItemRefUnchecked(int index) + { + Debug.Assert(_left != null && _right != null); + + if (index < _left._count) + { + return ref _left.ItemRefUnchecked(index); + } + + if (index > _left._count) + { + return ref _right.ItemRefUnchecked(index - _left._count - 1); } return ref _key; From ecfafac1c538878b190536f396a499911b88e680 Mon Sep 17 00:00:00 2001 From: Mansoor Saqib Date: Tue, 25 May 2021 21:44:08 -0700 Subject: [PATCH 2/4] Remove redundant code --- .../Collections/Immutable/ImmutableList_1.Node.cs | 12 +----------- .../Immutable/ImmutableSortedSet_1.Node.cs | 12 +----------- 2 files changed, 2 insertions(+), 22 deletions(-) diff --git a/src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList_1.Node.cs b/src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList_1.Node.cs index 7cde4fce30e8c6..a27c0ee1c2a2d7 100644 --- a/src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList_1.Node.cs +++ b/src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList_1.Node.cs @@ -195,19 +195,9 @@ internal T this[int index] internal ref readonly T ItemRef(int index) { Requires.Range(index >= 0 && index < this.Count, nameof(index)); - Debug.Assert(_left != null && _right != null); - if (index < _left._count) - { - return ref _left.ItemRefUnchecked(index); - } - if (index > _left._count) - { - return ref _right.ItemRefUnchecked(index - _left._count - 1); - } - - return ref _key; + return ref ItemRefUnchecked(index); } internal ref readonly T ItemRefUnchecked(int index) diff --git a/src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableSortedSet_1.Node.cs b/src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableSortedSet_1.Node.cs index ebd3940e4c1da6..6848ca9ce1bfab 100644 --- a/src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableSortedSet_1.Node.cs +++ b/src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableSortedSet_1.Node.cs @@ -263,17 +263,7 @@ internal ref readonly T ItemRef(int index) Requires.Range(index >= 0 && index < this.Count, nameof(index)); Debug.Assert(_left != null && _right != null); - if (index < _left._count) - { - return ref _left.ItemRefUnchecked(index); - } - - if (index > _left._count) - { - return ref _right.ItemRefUnchecked(index - _left._count - 1); - } - - return ref _key; + return ref ItemRefUnchecked(index); } internal ref readonly T ItemRefUnchecked(int index) From d6c873b21d5d0c4ee453180aaca09f1345a24487 Mon Sep 17 00:00:00 2001 From: Mansoor Saqib Date: Tue, 25 May 2021 22:05:48 -0700 Subject: [PATCH 3/4] Remove redundant assert --- .../src/System/Collections/Immutable/ImmutableList_1.Node.cs | 1 - .../System/Collections/Immutable/ImmutableSortedSet_1.Node.cs | 1 - 2 files changed, 2 deletions(-) diff --git a/src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList_1.Node.cs b/src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList_1.Node.cs index a27c0ee1c2a2d7..e243c9bab9c9fa 100644 --- a/src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList_1.Node.cs +++ b/src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList_1.Node.cs @@ -195,7 +195,6 @@ internal T this[int index] internal ref readonly T ItemRef(int index) { Requires.Range(index >= 0 && index < this.Count, nameof(index)); - Debug.Assert(_left != null && _right != null); return ref ItemRefUnchecked(index); } diff --git a/src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableSortedSet_1.Node.cs b/src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableSortedSet_1.Node.cs index 6848ca9ce1bfab..4501860b113166 100644 --- a/src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableSortedSet_1.Node.cs +++ b/src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableSortedSet_1.Node.cs @@ -261,7 +261,6 @@ internal T this[int index] internal ref readonly T ItemRef(int index) { Requires.Range(index >= 0 && index < this.Count, nameof(index)); - Debug.Assert(_left != null && _right != null); return ref ItemRefUnchecked(index); } From e7ea9a01637bf3e50a4f6f7fdf51570928aeb2de Mon Sep 17 00:00:00 2001 From: Mansoor Saqib Date: Fri, 28 May 2021 16:32:48 -0700 Subject: [PATCH 4/4] Apply suggestions from code review Change from internal to private for unchecked methods. Co-authored-by: Stephen Toub --- .../src/System/Collections/Immutable/ImmutableList_1.Node.cs | 2 +- .../System/Collections/Immutable/ImmutableSortedSet_1.Node.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList_1.Node.cs b/src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList_1.Node.cs index e243c9bab9c9fa..ac8efc506b439a 100644 --- a/src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList_1.Node.cs +++ b/src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList_1.Node.cs @@ -199,7 +199,7 @@ internal ref readonly T ItemRef(int index) return ref ItemRefUnchecked(index); } - internal ref readonly T ItemRefUnchecked(int index) + private ref readonly T ItemRefUnchecked(int index) { Debug.Assert(_left != null && _right != null); if (index < _left._count) diff --git a/src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableSortedSet_1.Node.cs b/src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableSortedSet_1.Node.cs index 4501860b113166..44d7f1768c6785 100644 --- a/src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableSortedSet_1.Node.cs +++ b/src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableSortedSet_1.Node.cs @@ -265,7 +265,7 @@ internal ref readonly T ItemRef(int index) return ref ItemRefUnchecked(index); } - internal ref readonly T ItemRefUnchecked(int index) + private ref readonly T ItemRefUnchecked(int index) { Debug.Assert(_left != null && _right != null);