-
Notifications
You must be signed in to change notification settings - Fork 357
perf: Reduce memory allocations using ReadOnlySpan and String interning #3360
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
base: main
Are you sure you want to change the base?
perf: Reduce memory allocations using ReadOnlySpan and String interning #3360
Conversation
106d285
to
25dc33e
Compare
break; | ||
|
||
case 3: // url, @id | ||
if (Is(span, ODataJsonConstants.ODataServiceDocumentElementUrlName)) |
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.
Do we care about case sensitivity in this case?
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.
Since we are checking the word or string as it comes, we just need to maintain case sensitivity.
{ | ||
switch (span.Length) | ||
{ | ||
case 2: // id, Id |
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.
I like the idea of this method, to avoid allocating strings from a span for common values (it doesn't seem limited to properties as the name of the methods suggest). I like that we check the length, so it's cheap to find the suitable bucket.
My concern is around the cases where we have length >= 5 and multiple candidates in the same block. This means we may have to do multiple string comparisons. I guess the obvious optimization here is to make sure that the most likely string candidate appears first in the if statement. Of course this can vary from one service to another. How are we sure this is optimal. I'd like to see what the cost of value of length 5+ is when it's not one of the list candidates (meaning that we would have to compare all candidates, i.e 9 * 5 = 45 characters) each time then fall back. I would like to compare the cost of doing that vs creating the string directly.
Another option would be to create static hashset of common words longer than some threshold (e.g. >= 5), the threshold can be picked by experimentation. Then do a lookup in the hashset and compare the cost of doing that to the cost of comparing all items. Not sure which would be faster. Could be worth the experiment, especially if this method is called each time we want to convert a span to a string. We could also try to replace the hashset with a simpler hash-based datastructure.
bb23156
to
e0ca212
Compare
/AzurePipelines run |
No pipelines are associated with this pull request. |
d655931
to
e21801c
Compare
/AzurePipelines run |
b238c80
to
55e24ea
Compare
/AzurePipelines run |
55e24ea
to
df0c7c6
Compare
df0c7c6
to
b2b13d6
Compare
https://github.com/OData/odata.net into fix/eliminate-avoidable-string-allocations-jsonreader
/AzurePipelines run |
Issues
This pull request fixes #3352.
Description
Trying to eliminate string allocations where possible using ReadOnlySpan and String interning
Benchmark
1. Before
2. After
Benchmark project
/WanjohiSammy/ODataJsonReaderBenchmarks-3
Input data:
https://github.com/WanjohiSammy/ODataJsonReaderBenchmarks-3/blob/main/ODataJsonReaderBenchmarks/SamplePayloadWithValues.json
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 run
to 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}/merge
where{prId}
is the ID of the PR.