Skip to content
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

Modify segmented list to grow by growth rate. #75756

Merged

Conversation

ToddGrun
Copy link
Contributor

@ToddGrun ToddGrun commented Nov 5, 2024

By modifying this growth rate, we can reduce the amount of waste in a SegmentedList in scenarios where it's final capacity isn't known beforehand.

A growth rate of 2x matches the current code behavior. A growth rate of just over 1 matches the growth rate (1 segment) in the other PR (#75708).

All growth rates benchmarked: 1.000001, 1.1, 1.25, 1.5, 2

Obviously, the single segment growth rate is a non-starter without allowing null segments (which is the approach the other PR took).

The 2x rate matches current behavior, and is really only measured as a baseline. From my reading of this chart, it looks like 1.1 is the best of these choices.

*** Long benchmarking results ***
image

A growth rate of 2x matches the current code behavior. A growth rate of just over 1 matches the growth rate (1 segment) in the other PR (dotnet#75708).

All growth rates benchmarked: 1.000001, 1.1, 1.25, 1.5, 2

Obviously, the single segment growth rate is a non-starter without allowing null segments (which is the approach the other PR took).

The 2x rate matches current behavior, and is really only measured as a baseline. From my reading of this chart, it looks like 1.1 is the best of these choices.
@ToddGrun ToddGrun requested a review from a team as a code owner November 5, 2024 21:16
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Nov 5, 2024
@@ -42,6 +42,8 @@ internal class SegmentedList<T> : IList<T>, IList, IReadOnlyList<T>
private static readonly SegmentedArray<T> s_emptyArray = new(0);
private static IEnumerator<T>? s_emptyEnumerator;

public static double SegmentGrowthRate { get; set; } = 2.0;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SegmentGrowthRate

Will get rid of this and just inline the desired value if we decide to move forward with this PR.

// Note that this check works even when _items.Length overflowed thanks to the (uint) cast
if ((uint)newCapacity > MaxLength)
newCapacity = MaxLength;
}

// If the computed capacity is still less than specified, set to the original argument.
// Capacities exceeding Array.MaxLength will be surfaced as OutOfMemoryException by Array.Resize.
Copy link
Member

@sharwell sharwell Nov 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Need to update the branch following this to ensure the array is still a multiple of the segment size unless it's less than a single segment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not really needed though, right? Seems like we should just respect the size they requested if it's larger than we would be resizing to otherwise.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AddRange can grow by more than one, but we still want to retain page alignment. We only need to allow non-alignment for small collections, and collections where the user set Capacity directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved the code around, should be resolved

…mantics to indicate the shift amount, not the growth rate
@ToddGrun
Copy link
Contributor Author

ToddGrun commented Nov 7, 2024

Here's the numbers for shifts of 2 and 3 (1.25 and 1.125 growth rates respectively) with the test modified to specify the exact segment count such that the AddExtraItem takes it from a completely full segment group to the next outer segment allocation.

The numbers are fairly close between the two shifts, with 1.25 performing better for full allocations and 1.125 performing better for "worst case" allocations, as expected. From my look over, 1.125 looks preferable, as it's looks like it does better when averaging out the best/worst cases for both wallclock time and allocations.

image

@ToddGrun ToddGrun requested a review from a team as a code owner November 7, 2024 18:31
@ToddGrun ToddGrun merged commit 3d0442b into dotnet:main Nov 11, 2024
28 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Nov 11, 2024
ToddGrun added a commit to ToddGrun/roslyn that referenced this pull request Nov 13, 2024
…aught by speedometer

The change in dotnet#75756 was incorrect when the existing number of items is between SegmentSize /2 and SegmentSize. In this case, the size of the newCapacity would end up as exactly the requested capacity, causing a potentially O(n^2) allocation growth pattern if caller was just increasing the requested capacity by one from it's current size.

The fix is just to handle that case directly, and if the existing size falls into that range, to simply set the desired newCapacity to the SegmentSize.
ToddGrun added a commit that referenced this pull request Nov 13, 2024
…aught by speedometer (#75895)

The change in #75756 was incorrect when the existing number of items is between SegmentSize / 2 (inclusive) and SegmentSize (exclusive). In this case, the size of the newCapacity would end up as exactly the requested capacity, causing a potentially O(n^2) allocation growth pattern if caller was just increasing the requested capacity by one from it's current size.

The fix is just to handle that case directly, and if the existing size falls into that range, to simply set the desired newCapacity to the SegmentSize.
@jjonescz jjonescz modified the milestones: Next, 17.13 P2 Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants