-
Notifications
You must be signed in to change notification settings - Fork 351
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
Performance refactor - elide async and await #2481
Performance refactor - elide async and await #2481
Conversation
Can we get some benchmarks for this change |
622c172
to
f76e74b
Compare
This comment was marked as resolved.
This comment was marked as resolved.
f76e74b
to
fd6cf38
Compare
…en in derived classes
I've re-run the "SerializationComparisons" benchmarks and now I see improvements in both performance and memory of up to 10%. Great work. Before
After
|
…ting top-level payloads
I added some comparison results for before and after and @habbes also run some benchmarks and shared the data in a comment |
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) |
Description
The following sections describe the nature of changes made to improve performance and memory usage
1. Trade off pure asynchronous semantics (where it makes sense) to circumvent the async state machine overhead
When implementing asynchronous support, there are methods that I changed to
async
such that even bonehead exceptions would be caught by the state machine for async methods and placed on the returned task.Those category of methods take the following form:
The
VerifyYadaYada
methods verifies for example that the stream is not disposed, or that a delta link is being written within a delta resource set, etc.Without the
async
keyword, the exception would be raised directly rather than get placed on the returned task.In this PR, I have dropped the
async
modifier from such methods for the following reasons:Task
objects, etc.) for a condition that will almost never occur. If it occurred, the exception will still be caught by a method with theasync
modifier higher up in the call stack.2. Use of local functions to circumvent the async state machine overhead
Another category of methods where I dropped the
async
modifier are those where the asynchronous logic is run conditionally. By placing the asynchronous logic in a local function, it becomes possible to elideasync
andawait
. We get a performance benefit especially where the asynchronous logic is almost never executed. Such methods take the following form:To avoid paying a higher cost when the condition is false, the rewritten method looks as follows:
3. Elide async/await where different branches of logic in a method makes it possible
This PR also elides
async
andawait
where each branch of logic in the asynchronous method consist of a single asynchronous method call. For example;We rewrite these category of methods as follows:
4. Dropped use of expensive methods defined in
TaskUtils
This PR also drops use of expensive methods defined in
TaskUtils
class -FollowOnSuccessWithTask
,FollowOnSuccessWith
, etc. Local functions that achieve the same purpose are introduced.5. Performance and memory usage improvements to particular methods defined in
TaskUtils
There are asynchronous methods that we support but the logic in such methods do not currently perform any asynchronous operations.
For example,
CreateODataResourceWriterAsync
currently only initializes the writer to use for writing OData resources (same logic asCreateODataResourceWriter
). It makes use of a methodGetTaskForSynchronousOperation
defined inTaskUtils
class. That method is used as follows:Where
CreateODataResourceWriterImplementation
is a synchronous method.I considered replacing all instances of
GetTaskForAsynchronousOperation
withTask.FromResult
but held back since theGetTaskForAsynchronousOperation
method does some exception handling and returns a faulted task for all exceptions exceptOutOfMemoryException
.The
GetTaskForAsynchronousOperation
method makes use of 2 helper methods (GetCompletedTask
andGetFaultedTask
).I did some benchmarking to see whether using
Task.FromResult
andTask.FromException
in the 2 helper methods would result into better performance and memory usage metrics.Below were the results:
From the above results, it's clear that the
GetCompletedTask
andGetFaultedTask
as currently implemented are expensive. UsingTaskCompletionSource
to create a task instance has a higher cost than callingTask.FromResult
.I have refactored the
GetTaskForSynchronousOperation
to make use of bothTask.FromResult
andTask.FromException
(where possible -Task.FromException
is not supported across all frameworks that OData supports)6. Introduction of overloads of particular methods that accept delegates as parameters to prevent capturing of state from the enclosing context
The following 4 methods accept delegates as a parameter. In a lot of cases where they are used, they capture state from the enclosing context and as a consequence the C# compiler automatically generates a private "display class" that results into inadvertent allocations.
Below are examples of scenarios where "display classes" are created:
Since the above methods are quite heavily used, I introduced
protected
overloads that help prevent capturing state from the enclosing context. ForWriteTopLevelPayloadAsync
method for example, I introduced the following overloads:By making use of the overloads, the "display classes" are eliminated together with the associated allocation overheads
No self-allocations in
ODataJsonLightResourceSerializer.WriteResourceSetContextUriAsync
Objects Allocations Report: Base vs Diff
Functions Allocations Report: Base vs Diff
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.