-
Notifications
You must be signed in to change notification settings - Fork 161
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
Set capacity for TruncatedCollection(IQueryable, int, boolean) overload #1183
Conversation
@microsoft-github-policy-service agree company="Microsoft" |
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) Note that for smaller lists (less than 16 pre-allocating will be slower or showing no effect) |
fixes #1185 |
{ | ||
var items = source.Take(checked(pageSize + 1)); |
There was a problem hiding this comment.
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)?
var items = source.Take(checked(pageSize + 1)); | |
var items = source.Take(Capacity); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, applied
src/Microsoft.AspNetCore.OData/Query/Container/TruncatedCollectionOfT.cs
Show resolved
Hide resolved
5e5acb6
to
9e4a4de
Compare
src/Microsoft.AspNetCore.OData/Query/Container/TruncatedCollectionOfT.cs
Show resolved
Hide resolved
src/Microsoft.AspNetCore.OData/Query/Container/TruncatedCollectionOfT.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.AspNetCore.OData/Query/Container/TruncatedCollectionOfT.cs
Show resolved
Hide resolved
Thank you @senioroman4uk for your pull request contribution. It looks like a great optimization. However, I don't know if applying the |
@senioroman4uk You need to update the public API files |
src/Microsoft.AspNetCore.OData/Query/Container/TruncatedCollectionOfT.cs
Show resolved
Hide resolved
src/Microsoft.AspNetCore.OData/Query/Container/TruncatedCollectionOfT.cs
Outdated
Show resolved
Hide resolved
@senioroman4uk please fix the PublicApiOut tests: |
Updated public api files |
@@ -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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
src/Microsoft.AspNetCore.OData/Query/Container/TruncatedCollectionOfT.cs
Show resolved
Hide resolved
src/Microsoft.AspNetCore.OData/Query/Container/TruncatedCollectionOfT.cs
Show resolved
Hide resolved
…tionOfT.cs Co-authored-by: John Gathogo <john.gathogo@gmail.com>
@gathogojr applied your suggestions, please give it another review when you have a chance |
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
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