This repository was archived by the owner on Jan 23, 2023. It is now read-only.
More enumeration perf fixes#10
Merged
dotnet-bot merged 8 commits intodotnet:masterfrom Nov 11, 2014
AArnott:MoreEnumerationPerfFixes
Merged
More enumeration perf fixes#10dotnet-bot merged 8 commits intodotnet:masterfrom AArnott:MoreEnumerationPerfFixes
dotnet-bot merged 8 commits intodotnet:masterfrom
AArnott:MoreEnumerationPerfFixes
Conversation
Avoid perf hit from acquiring a reusable stack object to enumerate a collection that is empty, and thus requires no stack object. This improves ImmutableSortedDictionary directly, and both ImmutableDictionary and ImmutableHashSet benefit as well since they use ImmutableSortedDictionary internally.
This cuts out the expensive Guid.op_Equality step from ThrowDisposedIfNotOwned.
Enumerating some immutable collections are necessarily slower than ordinary collections because they have to walk binary trees. But we needn't throw in locks into the enumerator and slow it down further in an attempt to claim enumerators are somehow thread-safe. They're not. And misusing them across threads concurrently would already lead to corruption. By removing the locks here, we've significantly improved performance of enumerating ImmutableSortedDictionary (and the other binary tree based collections). Even though the locks weren't contested, they still took a significant chunk of time. The loss of the locking means that technically if the enumerator is in use across multiple threads then now it's possible for a recycled Stack to still be in use on another thread, leading to data corruption for the possibly innocent subsequent user of that stack. Since we've already decided that immutable collections will not be considered as behind a security boundary (i.e. we won't treat such possibilities as security compromising to the core system), this seems quite reasonable for such an unlikely and unsupported pattern to carry this risk given the huge wins it has for all the supported cases.
If SecureObjectPool.NewId were to execute in a concurrent scenario where one thread actually was suspended while other threads were allowed to increment the counter enough to go through all 32-bits of values and the timing happened to be _just write_, we could theoretically return UnassignedId. This fixes it by looping until we get it right.
Author
|
Thanks @ellismg for the feedback. I've resolved and please review. I'm afraid the conversation history was erased when I did a rebase (gasp!) of my pull request branch. It turned out my branch had build and test issues because of the rebase to the dotnet organization so I had to compensate. |
Contributor
|
Looks good to me, Andrew. Thank you for taking the time to help me understand the changes. |
pgavlin
added a commit
to pgavlin/corefx
that referenced
this pull request
Sep 4, 2015
pgavlin
added a commit
to pgavlin/corefx
that referenced
this pull request
Sep 8, 2015
Add OS X support for Sockets and address CR feedback from dotnet#10.
Olafski
pushed a commit
to Olafski/corefx
that referenced
this pull request
Jun 15, 2017
Add repro instructions for ILC internal compiler errors
This file contains hidden or 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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
7 participants
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.
Substantial improvements to enumerating binary tree based collections.
The numbers below come from the EnumerationPerformance test and EnumerationPerformance_Empty test. They are still left checked into the test codebase for purposes of reviewing this pull request but may be removed prior to merging if desired.
Enumerating large collections: (2X faster)
BEFORE
00:00:02.6142165
00:00:02.5085870
00:00:02.4910253
AFTER
00:00:01.3295709
00:00:01.2909526
00:00:01.2643406
Enumerating empty collections: (10X faster)
BEFORE
00:00:00.0248775
00:00:00.0122959
00:00:00.0123267
AFTER
00:00:00.0085142
00:00:00.0011506
00:00:00.0011199