Perf optimization on ODataJsonResourceDeserializer #3456
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements performance optimizations for ODataJsonResourceDeserializer by converting several async methods from Task to ValueTask return types and implementing synchronous fast-path optimizations using IsCompletedSuccessfully checks to avoid async state machine overhead when operations complete synchronously.
Key Changes:
- Converted return types from
Task/Task<T>toValueTask/ValueTask<T>for several methods - Added synchronous fast-path checks using
IsCompletedSuccessfullyto avoid async overhead - Refactored async paths into local static helper functions
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 11 comments.
| File | Description |
|---|---|
src/Microsoft.OData.Core/Json/ODataJsonResourceDeserializer.cs |
Refactored multiple async methods to use ValueTask and added synchronous fast-path optimizations with IsCompletedSuccessfully checks |
test/UnitTests/Microsoft.OData.Core.Tests/Json/ODataJsonResourceDeserializerTests.cs |
Added .AsTask() calls to convert ValueTask to Task for compatibility with test assertion methods |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
KenitoInc
force-pushed
the
kemunga/perf-ODataJsonResourceDeserializer
branch
from
36bdede to
492b0ca
Compare
…github.com/KenitoInc/odata.net into kemunga/perf-ODataJsonResourceDeserializer
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
test/UnitTests/Microsoft.OData.Core.Tests/Json/ODataJsonResourceDeserializerTests.cs
Outdated
Show resolved
Hide resolved
| } | ||
| catch (ODataException ex) | ||
| { | ||
| return Task.FromException<object>(ex); |
There was a problem hiding this comment.
To be consistency with other parts of code, this should likely be Task.FromException(ex) without the generic parameter.
| return Task.FromException<object>(ex); | |
| return Task.FromException(ex); |
| /// | ||
| /// This method fills the ODataDelta(Deleted)Link.Target property if the id is found in the payload. | ||
| /// </remarks> | ||
| internal async Task ReadDeltaLinkTargetAsync(ODataDeltaLinkBase link) |
| /// | ||
| /// This method fills the ODataDelta(Deleted)Link.Source property if the id is found in the payload. | ||
| /// </remarks> | ||
| internal async Task ReadDeltaLinkSourceAsync(ODataDeltaLinkBase link) |
There was a problem hiding this comment.
We have the similar issue (change Task to use ValueTask). Do you think we should extend your PR (not just one PR) to other APIs?
3 participants
Issues
*This pull request fixes #3434 *
Description
- Eliminate unnecessary allocations, such as avoiding async state machine initialization when operations complete synchronously.
- Leverage ValueTask where appropriate to reduce GC pressure.
<style> </style>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.
Repository notes
Team members can start a CI build by adding a comment with the text
/AzurePipelines runto a PR. A bot may respond indicating that there is no pipeline associated with the pull request. This can be ignored if the build is triggered.Team members should not trigger a build this way for pull requests coming from forked repositories. They should instead trigger the build manually by setting the "branch" to
refs/pull/{prId}/mergewhere{prId}is the ID of the PR.