Improve performance for GetViewBetween and enumerating TreeSubSet #30921

acerbusace · 2018-07-09T17:53:50Z

Increases performance for SortedSet - GetViewBetween and enumerating TreeSubSet
This also, fixes #30821

VersionCheckImpl is not called within the constructor anymore
count does not get updated by VersionCheckImpl anymore
count is now updated within VersionCheckCount which is only called when retrieving Count property.
As a result count has been replaced with Count where necessary.
For TreeSubSet the enumerator now uses the Count of the parent set (entire tree) when creating the
stack. This results in a larger stack size, however it eliminates the need to get the count of TreeSubSet,
which drastically increases the performance.

The performance impacts can be seen below (benchmarkdotnet was used on release build of corefx).

Size of Set: 5000000
Original: original code base
Optimized: changed code base for performance improvements

SubSet of size 10000

Got a SubSet of size 10000 (2495000 - 2505000) (GetView)
Enumerated through the SubSet (EnumerateView)

SubSet of size 500000

Got a SubSet of size 1000000 (2000000 - 3000000) (GetView)
Enumerated through the SubSet (EnumerateView)

The reason enumerating through a view has similar runtimes, is because the count property in the
original code is updated when the view is gotten and since no changes occur to the set, count doesn't
need to be updated during the start of enumeration.

* VersionCheckImpl is not called within the constructor * count does not get updated by VersionCheckImpl * count is now updated within VersionCheckCount which is only called when retrieving Count property. As a result count has been replaced with Count inside TreeSubSet/SortedSet where nessessary. * For TreeSubSet the enumerator now uses the Count of the parent set (entire tree) when creating the stack. This results in a larger stack size, however it eliminates the need to get the count of TreeSubSet, which drastically increases the performance.

dnfclas · 2018-07-09T17:54:01Z

All CLA requirements met.

safern · 2018-07-30T16:21:04Z

src/System.Collections/src/System/Collections/Generic/SortedSet.cs

@@ -311,6 +311,8 @@ object ICollection.SyncRoot

        // Virtual function for TreeSubSet, which may need to update its count.
        internal virtual void VersionCheck() { }
+        internal virtual void VersionCheckCount() { }
+        internal virtual int TotalCount() { return Count; }


Why is this TotalCount wrapper around Count needed?

https://github.com/acerbusace/corefx/blob/6564eb78f7fbe1408c8dbd82a498bb572e017c0f/src/System.Collections/src/System/Collections/Generic/SortedSet.cs#L1955

TotalCount is being used to get the parent set's Count property (when enumerating with a TreeSubSet). This way, enumeration does not require the Count property of TreeSubSet, hence no extra time wasted to updating the Count property first.

If there is another more efficient way, I can make the necessary changes.

Ah I see. I got confused and thought you just introduced the parent's TotalCount, missed the overriden version.

safern · 2018-07-30T20:22:37Z

src/System.Collections/src/System/Collections/Generic/SortedSet.TreeSubSet.cs

+            {
+                VersionCheckImpl();
+
+                if (versionCount != _underlying.version)


Instead of adding this new versionCount variable which makes it confusing, it took me a little while to understand what was versionCount needed for. Instead we could change VersionCheck to have a bool parameter that is false by default and when that is true then update count?

i.e

internal override void VersionCheck(bool updateCount = false) => VersionCheckImpl(updateCount); private void VersionCheckImpl(bool updateCount) { Debug.Assert(_underlying != null); if (version != _underlying.version) { root = _underlying.FindRange(_min, _max, _lBoundActive, _uBoundActive); version = _underlying.version; if (updateCount) { ... } } }

and then from Count just call it with that bool to true? I think that way it will be more readable and will understand the intention of why we're doing that. I guess with that we would get the same perf improvements.

That sounds like a good idea. The only issue is that we have no way of knowing if the current Count property of TreeSubSet is up to date. As a result very time the Count property is accessed (either through within the code or by the end user), Count will always be forcefully updated.

How about adding the updateCount bool like you mentioned (still gets rid of the VersionCheckCount method) but also keeping versionCount variable and renaming it to something easier to understand?

How about adding the updateCount bool like you mentioned (still gets rid of the VersionCheckCount method) but also keeping versionCount variable and renaming it to something easier to understand?

That is true, I'm fine with keeping it the way it is, but please rename the variable to something easier to understand and add a comment in the new method just so that people that reads that understand why we need an extra variable and what method is doing.

acerbusace · 2018-07-30T20:58:00Z

https://github.com/acerbusace/corefx/blob/6564eb78f7fbe1408c8dbd82a498bb572e017c0f/src/System.Collections/src/System/Collections/Generic/SortedSet.TreeSubSet.cs#L191

I was also thinking, how about changing InOrderTreeWalk to use parent set's count property instead? Especially in the case when TreeSubSet's count property is initially 0 and we are trying to update it using InOrderTreeWalk. In the above case since the stack is created with size 0, the run time is a lot worse than just O(n).

safern · 2018-07-31T16:24:26Z

I was also thinking, how about changing InOrderTreeWalk to use parent set's count property instead? Especially in the case when TreeSubSet's count property is initially 0 and we are trying to update it using InOrderTreeWalk. In the above case since the stack is created with size 0, the run time is a lot worse than just O(n).

Wouldn't using the parent set's count property call: https://github.com/acerbusace/corefx/blob/6564eb78f7fbe1408c8dbd82a498bb572e017c0f/src/System.Collections/src/System/Collections/Generic/SortedSet.TreeSubSet.cs#L317 ? And potentially if something changes the _underlying.Version in between we could end up in an infinite recursion calling InOrderTreeWalk again and going back and forth?

safern · 2018-07-31T17:39:40Z

src/System.Collections/src/System/Collections/Generic/SortedSet.TreeSubSet.cs

+                    if (updateCount) {
+                        count = 0;
+                        InOrderTreeWalk(n => { count++; return true; });
+                        countVersion = _underlying.version;


I don't see where this variable is used now. Shouldn't this be outside of the version != _underlying.version comparing countVersion to _underlying.version if updateCount is true?

acerbusace · 2018-07-31T17:40:42Z

Wouldn't using the parent set's count property call: https://github.com/acerbusace/corefx/blob/6564eb78f7fbe1408c8dbd82a498bb572e017c0f/src/System.Collections/src/System/Collections/Generic/SortedSet.TreeSubSet.cs#L317 ? And potentially if something changes the _underlying.Version in between we could end up in an infinite recursion calling InOrderTreeWalk again and going back and forth?

That is true. Let's leave that performance improvement for another time, then.

safern · 2018-07-31T17:47:57Z

src/System.Collections/src/System/Collections/Generic/SortedSet.cs

@@ -281,7 +281,7 @@ public int Count
        {
            get
            {
-                VersionCheck();
+                VersionCheckCount(true);


One last thing, sorry for the back and forth iterations, but for bool parameters we like to name them so that when you read this statement is clear and you don't have to go to the method declaration to see what that true stands for, i.e:

VersionCheck(updateCount: true);

and just noticed you're calling VersionCheckCount still, it should now be VersionCheck.

Also, would you mind re running your benchmarks just to make sure we're still getting the perf benefits?

Fixed that above issue and sure, I'll rerun the benchmarks.

The performance benefits are still there. Tested for Set of size 5000000 and SubSet (View) of Size 500000.

Nice! Thanks for the update!

safern · 2018-08-01T16:57:14Z

src/System.Collections/src/System/Collections/Generic/SortedSet.TreeSubSet.cs

+            // keeps track of whether the count variable is up to date
+            // up to date -> countVersion = _underlying.version
+            // not up to date -> countVersion < _underlying.version
+            private int countVersion;


Could we name this with _ I think the reason why version doesn't have the prefix is for serialization compatibility, since this is a new field we're adding, we can add the prefix.

safern · 2018-08-01T16:57:23Z

src/System.Collections/src/System/Collections/Generic/SortedSet.TreeSubSet.cs

            {
                Debug.Assert(_underlying != null);
                if (version != _underlying.version)
                {
                    root = _underlying.FindRange(_min, _max, _lBoundActive, _uBoundActive);
                    version = _underlying.version;
+                }
+
+                if (updateCount && countVersion != _underlying.version) {


Last thing, the opening bracket needs to be in a new line.

safern

LGTM. Thanks @acerbusace

…tnet/corefx#30921) * Improve performance for GetViewBetween and enumerating TreeSubSet * VersionCheckImpl is not called within the constructor * count does not get updated by VersionCheckImpl * count is now updated within VersionCheckCount which is only called when retrieving Count property. As a result count has been replaced with Count inside TreeSubSet/SortedSet where nessessary. * For TreeSubSet the enumerator now uses the Count of the parent set (entire tree) when creating the stack. This results in a larger stack size, however it eliminates the need to get the count of TreeSubSet, which drastically increases the performance. * Cleaned up code and added comments * Fixed logic error with updateCount * Removed unnecessary blank line * Changed VersionCheckCount -> VersionCheck * Cleaning code Commit migrated from dotnet/corefx@cc4d621

karelz added this to the 3.0 milestone Jul 19, 2018

joshfree added the area-System.Collections label Jul 20, 2018

joshfree assigned safern Jul 20, 2018

danmoseley requested a review from safern July 24, 2018 02:58

safern reviewed Jul 30, 2018

View reviewed changes

Cleaned up code and added comments

1df5a13

safern reviewed Jul 31, 2018

View reviewed changes

acerbusace added 2 commits July 31, 2018 13:44

Fixed logic error with updateCount

c3bf9da

Removed unnecessary blank line

d003e94

safern reviewed Jul 31, 2018

View reviewed changes

Changed VersionCheckCount -> VersionCheck

58a05ef

safern reviewed Aug 1, 2018

View reviewed changes

Cleaning code

dfeddb1

safern approved these changes Aug 1, 2018

View reviewed changes

safern merged commit cc4d621 into dotnet:master Aug 2, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve performance for GetViewBetween and enumerating TreeSubSet #30921

Improve performance for GetViewBetween and enumerating TreeSubSet #30921

acerbusace commented Jul 9, 2018

dnfclas commented Jul 9, 2018 •

edited

safern Jul 30, 2018

acerbusace Jul 30, 2018

safern Jul 30, 2018

safern Jul 30, 2018 •

edited

acerbusace Jul 30, 2018

safern Jul 31, 2018 •

edited

acerbusace commented Jul 30, 2018

safern commented Jul 31, 2018

safern Jul 31, 2018

acerbusace commented Jul 31, 2018

safern Jul 31, 2018

acerbusace Jul 31, 2018

acerbusace Aug 1, 2018

safern Aug 1, 2018

safern Aug 1, 2018

safern Aug 1, 2018

safern left a comment

Improve performance for GetViewBetween and enumerating TreeSubSet #30921

Improve performance for GetViewBetween and enumerating TreeSubSet #30921

Conversation

acerbusace commented Jul 9, 2018

dnfclas commented Jul 9, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

safern Jul 30, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

safern Jul 31, 2018 • edited

Choose a reason for hiding this comment

acerbusace commented Jul 30, 2018

safern commented Jul 31, 2018

Choose a reason for hiding this comment

acerbusace commented Jul 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

safern left a comment

Choose a reason for hiding this comment

dnfclas commented Jul 9, 2018 •

edited

safern Jul 30, 2018 •

edited

safern Jul 31, 2018 •

edited