-
Notifications
You must be signed in to change notification settings - Fork 352
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
Skip lambda allocation in FindTransientAnnotation #2154
Skip lambda allocation in FindTransientAnnotation #2154
Conversation
src/Microsoft.OData.Edm/Vocabularies/Annotations/EdmDirectValueAnnotationsManager.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.OData.Edm/Vocabularies/Annotations/EdmDirectValueAnnotationsManager.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.OData.Edm/Vocabularies/Annotations/EdmDirectValueAnnotationsManager.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.OData.Edm/Vocabularies/Annotations/EdmDirectValueAnnotationsManager.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.OData.Edm/Vocabularies/Annotations/EdmDirectValueAnnotationsManager.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.OData.Edm/Vocabularies/Annotations/EdmDirectValueAnnotationsManager.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.OData.Edm/Vocabularies/Annotations/EdmDirectValueAnnotationsManager.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.OData.Edm/Vocabularies/Annotations/EdmDirectValueAnnotationsManager.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.OData.Edm/Vocabularies/Annotations/EdmDirectValueAnnotationsManager.cs
Outdated
Show resolved
Hide resolved
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.
LG
src/Microsoft.OData.Edm/Vocabularies/Annotations/EdmDirectValueAnnotationsManager.cs
Outdated
Show resolved
Hide resolved
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
@@ -68,7 +68,7 @@ public override int Count | |||
|
|||
public override VersioningList<TElement> Add(TElement value) | |||
{ | |||
return new LinkedVersioningList(this, value); | |||
return new ArrayVersioningList(this, value); |
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.
If I understand the implementation, this results in a new array being allocated, and elements copied from the previous array to the new one. Doesn't this lead to a larger memory footprint and more GC over time? Is the rationale that we're willing to sacrifice memory in order to get faster search times?
Issues
Total allocation of this method is at 3.09%:
Description
This change replaces the usage of Linq in this method, with manual implementation, avoiding lambda allocation. Every time we invoke Any, it allocates a lambda. This is normally meaningless, when not present in the hot path. Since this is present in AGS hot path, it is costing 1% of allocations just in this lambda. This is a lot.
Also removes the usage of enumerator, replacing with O(1) random access for loop. The ArrayVersioningList is better for search scenarios because we can save the enumerator allocation all together, and use O(1) random access. The LinkedVersioningList would only be useful if this data structure is changing very often at runtime, which I don't think is the case? Main scenario will be search at runtime? So I am proposing to replace with ArrayVersioningList. Furthermore, when adding a new element, it was swapping to ArrayVersioningList if there were 5 or more elements, hence I don't really see the benefit of not using ArrayVersioningList for all cases, particular if the search scenario is the main path.
Not using Linq.Any(), is not only good for memory but also for CPU as we can see in this benchmark:
Checklist (Uncheck if it is not completed)