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

Set capacity for TruncatedCollection(IQueryable, int, boolean) overload #1183

Merged

Conversation

senioroman4uk
Copy link
Contributor

@senioroman4uk senioroman4uk commented Mar 2, 2024

After profiling our service that uses AspNetCore.OData with a huge page size of 3000. I've noticed a lot of time is spent inside TruncatedCollection overload for IQueryable
image

Since it uses List as a base class current initialization will resize quite a few times before reaching the page size. This PR attempts to fix this for some of the overloads were size of the page is known

fixes #1185

@senioroman4uk
Copy link
Contributor Author

@microsoft-github-policy-service agree company="Microsoft"

@senioroman4uk senioroman4uk marked this pull request as draft March 2, 2024 05:18
@senioroman4uk
Copy link
Contributor Author

here are the results of the test I did with a sample server and bombardier

Bombarding https://localhost:7034/odata/Products for 1m0s using 10 connection(s)
Before
image

After
image

Note that for smaller lists (less than 16 pre-allocating will be slower or showing no effect)

@senioroman4uk
Copy link
Contributor Author

fixes #1185

{
var items = source.Take(checked(pageSize + 1));
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to do something like this to avoid the same calculation again (and opening it up for potential inconsistencies in the future)?

Suggested change
var items = source.Take(checked(pageSize + 1));
var items = source.Take(Capacity);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, applied

@senioroman4uk senioroman4uk force-pushed the senioroman4uk/fix-truncated-collection branch from 5e5acb6 to 9e4a4de Compare March 4, 2024 21:49
@senioroman4uk senioroman4uk marked this pull request as ready for review March 5, 2024 00:16
@gathogojr
Copy link
Contributor

Thank you @senioroman4uk for your pull request contribution. It looks like a great optimization. However, I don't know if applying the Obsolete attribute to some of the constructors without a good understanding of the use cases they were intended for was a good idea since that action introduced extra things/changes to debate about. I hope it won't cause unnecessary delays.

@gathogojr
Copy link
Contributor

@senioroman4uk You need to update the public API files

habbes
habbes previously approved these changes Mar 8, 2024
@habbes
Copy link
Contributor

habbes commented Mar 8, 2024

@senioroman4uk please fix the PublicApiOut tests:

image

@senioroman4uk senioroman4uk dismissed stale reviews from habbes and xuzhg via 136a7b6 March 8, 2024 21:05
@senioroman4uk
Copy link
Contributor Author

Updated public api files

@senioroman4uk
Copy link
Contributor Author

@habbes @xuzhg, can you please re-review I have addressed your suggestions

@@ -17,6 +17,9 @@ namespace Microsoft.AspNetCore.OData.Query.Container
/// <typeparam name="T">The collection element type.</typeparam>
public class TruncatedCollection<T> : List<T>, ITruncatedCollection, IEnumerable<T>, ICountOptionCollection
{
// The default capacity of the list.
// https://github.com/microsoft/referencesource/blob/master/mscorlib/system/collections/generic/list.cs#L38
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a major issue, I think this is .NET Framework's code. Might be more relevant to reference the current .NET's code: https://github.com/dotnet/runtime/blob/main/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/List.cs#L23

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

habbes
habbes previously approved these changes Mar 19, 2024
habbes
habbes previously approved these changes Mar 20, 2024
…tionOfT.cs

Co-authored-by: John Gathogo <john.gathogo@gmail.com>
@senioroman4uk
Copy link
Contributor Author

@gathogojr applied your suggestions, please give it another review when you have a chance

@habbes habbes merged commit 157e4ce into OData:main Mar 26, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Truncated Collection causes list resizing when endpoint has a huge page size
5 participants