Reduce memory and CPU costs due to SegmentedList usage #75661
Merged
Conversation
This file contains 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
Currently, the SegmentedList class suffers from two inefficiencies: 1) Upon growth, it doubles the SegmentedArray size. This is necessary for normal List like collections to get constant time amortized growth, but isn't necessary (or desirable) for segmented data structures. 2) Upon growth, it reallocates and copies over the existing pages. Instead, if we only allocate the modified/new pages and the array holding the pages, we can save significant CPU and allocation costs.
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
ToddGrun
commented
Oct 29, 2024
|
Doubling makes sense when you only have a single backing store, and exceeding its limits causes another allocation and full copy. Given that we're already segmented, and growth shouldn't cause copies of prior segments, this seems like a sensible change to me. Would like perf numbers if possible though. |
|
Oh. I didn't scroll down far enough. |
CyrusNajmabadi
approved these changes
Oct 29, 2024
CyrusNajmabadi
approved these changes
Oct 29, 2024
sharwell
reviewed
Oct 29, 2024
sharwell
reviewed
Oct 29, 2024
src/Tools/IdeCoreBenchmarks/SegmentedListBenchmarks_InsertRange.cs
Outdated
Show resolved
Hide resolved
sharwell
reviewed
Oct 31, 2024
src/Dependencies/Collections/SegmentedArray`1+PrivateMarshal.cs
Outdated
Show resolved
Hide resolved
sharwell
reviewed
Oct 31, 2024
sharwell
reviewed
Oct 31, 2024
sharwell
reviewed
Oct 31, 2024
sharwell
reviewed
Oct 31, 2024
sharwell
reviewed
Oct 31, 2024
| @@ -502,7 +529,17 @@ internal void Grow(int capacity) | |||
| // If the computed capacity is still less than specified, set to the original argument. | |||
There was a problem hiding this comment.
Requested changes:
- Document the algorithm for initialization of
newCapacity - For initial length greater than or equal to half the segment size but less than one full segment size, set
newCapacityto one segment size. - For any initial length where the final segment is not a full segment, set
newCapacityto the length it would be with a full-size final segment. This guarantees that the outer array will not be reallocated, and also guarantees that the single inner array allocation performed during the resize will not need to be performed a second time during the next resize. (Can be modified and treated as a generalization of the preceding point) - If the calculated
newCapacityends up being less thancapacityandcapacityis greater than the segment size, apply a final ceiling operation so the final segment is full size.
These rules are all relevant regardless of whether we increase by doubling or by a page at a time. I am still reviewing the choice of expansion size.
There was a problem hiding this comment.
➡️ Changing the capacity selection algorithm for resize has been deferred to a later pull request.
sharwell
reviewed
Oct 31, 2024
sharwell
reviewed
Oct 31, 2024
src/Dependencies/Collections/SegmentedArray`1+PrivateMarshal.cs
Outdated
Show resolved
Hide resolved
…move need for a computation of the length
sharwell
approved these changes
Nov 1, 2024
This was referenced Nov 4, 2024
ToddGrun
added a commit
to ToddGrun/roslyn
that referenced
this pull request
Nov 16, 2024
Mimics changes done in dotnet#75661 to enable segment reuse in SegmentedDictionary.
ToddGrun
added a commit
that referenced
this pull request
Nov 21, 2024
* Reuse segments during SegmentedDictionary growth Mimics changes done in #75661 to enable segment reuse in SegmentedDictionary.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Currently, the SegmentedList class suffers from two potential inefficiencies:
This PR addresses 2).
Instead, if we reuse existing segments when growing the list, we can save significant CPU and allocation costs.
*** Benchmark.NET data ***
OLD:
NEW:
