-
Notifications
You must be signed in to change notification settings - Fork 350
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
Reduce memory allocations from closures and lambdas #2197
Conversation
97aa8ed
to
488e33a
Compare
test/PerformanceTests/SerializationBaselineTests/SerializationBaselineTests.csproj
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) |
Issues
This pull request fixes #2192 .
Description
Optimizes the hot spots specified in the issue description and also adds new benchmarks to the performance tests solution.
ODataWriterCore.CheckForNestedResourceInfoWithContentAsync
ExtensionMethods.FindType
list.FirstOrDefault
with for loop to avoid closure allocations inEdmExtensions.ReplaceAlias
DerivedTypeConstraints.Any()
withforeach
loop inWriterValidationUtils.ValidatePropertyDerivedTypeConstraints
. Usingforeach
does cause anIEnumerator
to be allocated on the heap, but so does usingAny()
in this case (since the underlyingIEnumerable
is not a collection with a cheap count). There's a separate issue that aims to reduceIEnumerator
allocations, and that would involve checking whether this collection can be converted to aList<T>
. I will do that investigation when dealing with that issue.derivedTypeConstraints.Any()
withforeach
loop inWriterValidationUtils.ValidateDerivedTypeConstraints
. (Same remarks as the item above)this.children.Any()
withthis.children.TryGetValue
inSelectedPropertiesNode.GetSelectedPropertiesForNavigationProperty
. Also replace thethis.GetMatchinTypeSegments().Select()
with a methodGetSelectedPropertieForTypeSegmentsNavigationProperty
that encapsulates that does the same thing as theSelect()
but without lambdas.I've also added a new benchmarks project. The purpose of these benchmarks is to compare performance between the JsonSerializer and ODataWriter (both sync and async). These are based on the experiments that I've been running locally to uncover issues with writing. The pre-existing benchmarks did not cover async code paths and do not give us insight into how we're doing compared System.Text.Json or some other "standard" serialization library.
Benchmarks
On my local profile, the number of total allocations from
Microsoft.OData.Core
assembly went down from 1,475,609 to 1,385,600 (About 6% reduction).The benchmarks below were run before and after the set of changes. They indicate a reduction of about 3.2-3.4% in allocated memory for the synchronous ODataWriter, and about 1.4% reduction for the async ODataWriter.
There also seems to be consistent reduction in run time, but I'm not sure to which extent this reduction is significant.
These benchmarks have been added to the repo and you can run them using (not sure if this was the best name of the benchmarks):
Before
After
Here are the pre-existing
ODataWriter
benchmarks:Before
After
Checklist (Uncheck if it is not completed)
Additional work necessary
If documentation update is needed, please add "Docs Needed" label to the issue and provide details about the required document change in the issue.
cc @joaocpaiva