Perf optimization on ODataJsonResourceDeserializer#3456
Perf optimization on ODataJsonResourceDeserializer#3456KenitoInc wants to merge 35 commits intoOData:mainfrom
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?
|
@KenitoInc This is a perf-improvement pull request so I started review by analyzing the benchmarks you attached and the results I got were a bit mixed.
Allocations and GC look essentially flat across all cases. Given that this refactor adds complexity primarily for performance, it’s hard to justify where the effect is negligible (≤~3%) or regressive on important configurations (notably ServerGC with Base/Large payloads). The ServerGC regressions in particular are concerning for throughput-oriented workloads. I had a similar experience with a perf pull request targeted at Suggestions to move forward: Given that we can’t realistically maintain divergent logic paths based on GC mode or payload shape, the most productive next steps are about understanding variance and validating that we’re measuring the right thing, rather than further refactoring at this stage.
Overall, tightening our understanding of the ServerGC behavior and firming up the benchmark signal should put us in a much better position to decide whether further complexity is justified, or whether the current refactor should be reconsidered or simplified. |
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.