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

Fixed performance of Min/Max in SortedSet. #11968

Merged
merged 2 commits into from Sep 23, 2016

Conversation

Projects
None yet
4 participants
@mjp41
Member

mjp41 commented Sep 22, 2016

The existing implementation of Min/Max calls InOrderTreeWalk,
which perform an allocation of a stack for traversing the tree.
But when you are finding the smallest or largest element in a
tree there is no need to perform this allocation. This is an
optimised version that directly finds the smallest or largest
element.

This is an important use case for using a sorted set as a priority
queue. In microbenchmarking, with the following code I
found a 30-40% speed up:

        for(int j=0; j < 10; j++) {
            var s = new SortedSet<object>();
            for(int n = 0; n < 100000; n++) {
                s.Add(n);
            }

            int i = 0;
            while(s.Min != null) 
            {
                var o = s.Min;
                s.Remove(o);
                i++;
                if(i%2 == 0) {
                    s.Add((int)o * 2);
                }
            }
        }
Fixed performance of Min/Max in SortedSet.
The existing implementation of Min/Max calls InOrderWalkTree,
which perform an allocation of a stack for traversing the tree.
But when you are finding the smallest or largest element in a
tree there is no need to perform this allocation.  This is an
optimised version that directly finds the smallest or largest
element.
@stephentoub

Other than the two nits, LGTM. Thanks for finding/fixing this, @mjp41.

@mjp41

This comment has been minimized.

Show comment
Hide comment
@mjp41

mjp41 Sep 22, 2016

Member

Thanks @stephentoub, I have fixed the code review feedback.

There is a similar problem with SortedDictionary, but it is much harder to fix as I think it requires an API change. I'll raise an issue for this, as the choices need discussing.

Member

mjp41 commented Sep 22, 2016

Thanks @stephentoub, I have fixed the code review feedback.

There is a similar problem with SortedDictionary, but it is much harder to fix as I think it requires an API change. I'll raise an issue for this, as the choices need discussing.

@stephentoub

This comment has been minimized.

Show comment
Hide comment
@stephentoub

stephentoub Sep 22, 2016

Member

@dotnet-bot Test Innerloop CentOS7.1 Release Build and Test please ("Failed to mkdirs")

Member

stephentoub commented Sep 22, 2016

@dotnet-bot Test Innerloop CentOS7.1 Release Build and Test please ("Failed to mkdirs")

@stephentoub stephentoub merged commit 9c416bc into dotnet:master Sep 23, 2016

9 of 10 checks passed

Innerloop CentOS7.1 Release Build and Test Build finished.
Details
Innerloop CentOS7.1 Debug Build and Test Build finished.
Details
Innerloop Linux ARM Emulator Debug Cross Build Build finished.
Details
Innerloop Linux ARM Emulator Release Cross Build Build finished.
Details
Innerloop OSX Debug Build and Test Build finished.
Details
Innerloop OSX Release Build and Test Build finished.
Details
Innerloop Ubuntu14.04 Debug Build and Test Build finished.
Details
Innerloop Ubuntu14.04 Release Build and Test Build finished.
Details
Innerloop Windows_NT Debug Build and Test Build finished.
Details
Innerloop Windows_NT Release Build and Test Build finished.
Details

@karelz karelz modified the milestone: 1.2.0 Dec 3, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment