Performance improvements #2215

henning-krause · 2021-10-10T19:00:30Z

Issues

This pull request partly addresses #2182.

Description

Based on this performance experiments I fixed some small performance issues:

Replaced a few "async Task" methods with simple "Task" methods to remove the async overhead
Made a few allocations lazy to avoid creating variable members when they aren't needed.
Removed the necessity to use a class based enumerator in two instances

I ran the benchmark provided by @habbes before and after my change. These are the results:

Method	Job	UnrollFactor	dataSize	Mean	Error	StdDev	Median	Ratio	RatioSD	Gen 0	Gen 1	Gen 2	Allocated
WriteJson Before	DefaultJob	16	1000	1.480 ms	0.0296 ms	0.0598 ms	1.488 ms	1.00	0.00	109.3750	109.3750	109.3750	447.45 KB
WriteJson After	DefaultJob	16	1000	1.549 ms	0.0309 ms	0.0289 ms	1.555 ms	1.00	0.00	109.3750	109.3750	109.3750	525.53 KB
WriteOData Before	DefaultJob	16	1000	51.163 ms	1.0229 ms	1.2176 ms	51.309 ms	33.22	0.91	1000.0000	-	-	16703.48 KB
WriteOData After	DefaultJob	16	1000	42.685 ms	0.8428 ms	1.7405 ms	41.901 ms	28.91	1.64	1000.0000	-	-	12773.67 KB
WriteODataSync Before	DefaultJob	16	1000	23.373 ms	0.2427 ms	0.2027 ms	23.494 ms	15.08	0.35	1843.7500	250.0000	93.7500	15015.88 KB
WriteODataSync After	DefaultJob	16	1000	19.313 ms	0.5337 ms	1.5482 ms	19.127 ms	13.12	1.30	1281.2500	125.0000	93.7500	11085.11 KB

WriteJsonFile Before	Job-CRROQW	1	1000	3.363 ms	0.0660 ms	0.0706 ms	3.353 ms	?	?	-	-	-	82.26 KB
WriteJsonFile After	Job-NSTZLY	1	1000	2.871 ms	0.2269 ms	0.6654 ms	2.620 ms	?	?	-	-	-	2.96 KB
WriteODataFile Before	Job-CRROQW	1	1000	52.491 ms	0.6490 ms	0.6071 ms	52.739 ms	?	?	1000.0000	-	-	16149.03 KB
WriteODataFile After	Job-NSTZLY	1	1000	48.463 ms	1.3789 ms	4.0004 ms	47.185 ms	?	?	1000.0000	-	-	12220.95 KB
WriteODataSyncFile Before	Job-CRROQW	1	1000	22.262 ms	0.4416 ms	0.8076 ms	22.135 ms	?	?	1000.0000	1000.0000	-	14508.67 KB
WriteODataSyncFile After	Job-NSTZLY	1	1000	20.527 ms	0.9368 ms	2.6878 ms	19.716 ms	?	?	1000.0000	1000.0000	-	10578.11 KB

WriteJson Before	DefaultJob	16	5000	8.340 ms	0.0457 ms	0.0427 ms	8.346 ms	1.00	0.00	796.8750	750.0000	750.0000	4069.07 KB
WriteJson After	DefaultJob	16	5000	6.362 ms	0.0185 ms	0.0173 ms	6.358 ms	1.00	0.00	726.5625	718.7500	718.7500	3676.58 KB
WriteOData Before	DefaultJob	16	5000	272.201 ms	4.3834 ms	4.1003 ms	272.966 ms	32.64	0.43	9000.0000	1000.0000	-	83128.66 KB
WriteOData After	DefaultJob	16	5000	204.872 ms	0.7933 ms	0.7421 ms	204.995 ms	32.20	0.12	7000.0000	1000.0000	-	63525.25 KB
WriteODataSync Before	DefaultJob	16	5000	110.368 ms	2.1764 ms	5.0442 ms	111.393 ms	12.44	0.35	8000.0000	-	-	76540.07 KB
WriteODataSync After	DefaultJob	16	5000	80.544 ms	0.6720 ms	0.5247 ms	80.333 ms	12.66	0.10	7000.0000	714.2857	571.4286	56890.49 KB

WriteJsonFile Before	Job-CRROQW	1	5000	12.128 ms	0.6244 ms	1.8410 ms	11.207 ms	?	?	-	-	-	410.89 KB
WriteJsonFile After	Job-NSTZLY	1	5000	8.785 ms	0.1496 ms	0.2987 ms	8.701 ms	?	?	-	-	-	13.77 KB
WriteODataFile Before	Job-CRROQW	1	5000	268.124 ms	5.1947 ms	6.5697 ms	270.428 ms	?	?	9000.0000	1000.0000	-	80688.21 KB
WriteODataFile After	Job-NSTZLY	1	5000	203.037 ms	0.9306 ms	0.8705 ms	203.052 ms	?	?	7000.0000	1000.0000	-	61075.26 KB
WriteODataSyncFile Before	Job-CRROQW	1	5000	120.416 ms	2.3963 ms	5.4089 ms	122.195 ms	?	?	8000.0000	1000.0000	-	72448.74 KB
WriteODataSyncFile After	Job-NSTZLY	1	5000	81.773 ms	1.0918 ms	0.9117 ms	82.001 ms	?	?	6000.0000	1000.0000	-	52798.7 KB

WriteJson Before	DefaultJob	16	10000	16.155 ms	0.0489 ms	0.0458 ms	16.153 ms	1.00	0.00	843.7500	750.0000	750.0000	8148.33 KB
WriteJson After	DefaultJob	16	10000	12.623 ms	0.0394 ms	0.0349 ms	12.624 ms	1.00	0.00	750.0000	750.0000	750.0000	7366.2 KB
WriteOData Before	DefaultJob	16	10000	547.669 ms	5.2592 ms	4.6622 ms	546.506 ms	33.90	0.29	19000.0000	1000.0000	-	166335.63 KB
WriteOData After	DefaultJob	16	10000	420.105 ms	8.3829 ms	11.4746 ms	415.970 ms	33.81	0.92	14000.0000	1000.0000	-	127209.09 KB
WriteODataSync Before	DefaultJob	16	10000	228.369 ms	4.5231 ms	9.9283 ms	227.668 ms	13.73	0.70	17000.0000	-	-	153061.27 KB
WriteODataSync After	DefaultJob	16	10000	153.988 ms	1.6143 ms	1.3480 ms	153.594 ms	12.19	0.12	12000.0000	-	-	113761.68 KB

WriteJsonFile Before	Job-CRROQW	1	10000	21.888 ms	0.4346 ms	1.0329 ms	21.645 ms	?	?	-	-	-	816.33 KB
WriteJsonFile After	Job-NSTZLY	1	10000	17.841 ms	0.2407 ms	0.2251 ms	17.769 ms	?	?	-	-	-	22.19 KB
WriteODataFile Before	Job-CRROQW	1	10000	548.185 ms	6.1334 ms	5.4371 ms	546.844 ms	?	?	19000.0000	1000.0000	-	161366.68 KB
WriteODataFile After	Job-NSTZLY	1	10000	414.704 ms	1.7958 ms	1.6798 ms	414.285 ms	?	?	14000.0000	1000.0000	-	122237.91 KB
WriteODataSyncFile Before	Job-CRROQW	1	10000	231.648 ms	4.5946 ms	7.9254 ms	233.283 ms	?	?	17000.0000	-	-	144873.92 KB
WriteODataSyncFile After	Job-NSTZLY	1	10000	168.528 ms	2.2990 ms	2.1505 ms	168.644 ms	?	?	12000.0000	-	-	105574.34 KB

Checklist (Uncheck if it is not completed)

Test cases added
Build and test with one-click build and test script passed

Additional work necessary

* Replaced a few "async Task" methods with simple "Task" methods to remove the async overhead * Made a few allocations lazy to avoid creating variable members when they aren't needed. * Removed the necessity to use a class based enumerator in two instances

src/Microsoft.OData.Core/InstanceAnnotationWriteTracker.cs

henning-krause · 2021-10-25T16:49:11Z

@habbes - Done

src/Microsoft.OData.Core/InstanceAnnotationWriteTracker.cs

odero · 2021-11-08T13:28:29Z

@henning-krause Thanks for the update. Good to see these results
If you don't mind could you merge the before and after into 1 table - makes for easier reading during comparison

cc @gathogojr you might want to look at this if you haven't already

henning-krause · 2021-11-08T16:42:34Z

@odero I've reverted the requested file and merged the table in my first post.

gathogojr · 2021-11-08T19:11:36Z

@henning-krause Thanks for the update. Good to see these results If you don't mind could you merge the before and after into 1 table - makes for easier reading during comparison

cc @gathogojr you might want to look at this if you haven't already

Noted. Will take a look

src/Microsoft.OData.Core/Json/JsonValueUtilsAsync.cs

src/Microsoft.OData.Core/Json/JsonWriterAsync.cs

src/Microsoft.OData.Core/JsonLight/ODataJsonLightPropertySerializer.cs

src/Microsoft.OData.Core/ODataAnnotatable.cs

src/Microsoft.OData.Core/ODataWriterCore.cs

src/Microsoft.OData.Core/ODataAnnotatable.cs

src/Microsoft.OData.Core/Json/JsonValueUtilsAsync.cs

mikepizzo · 2022-01-03T23:13:31Z

Thanks @henning-krause for the contribution!

I added a few minor comments

If we can get this rebased, and comments addressed, I'd love to merge it for our next release.

mikepizzo

🕐

mikepizzo

pull-request-quantifier-deprecated · 2022-01-04T16:59:14Z

This PR has 68 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

Quantification details

Label      : Small
Size       : +31 -37
Percentile : 27.2%

Total files changed: 5

Change summary by file extension:
.cs : +31 -37

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Why proper sizing of changes matters

Optimal pull request sizes drive a better predictable PR flow as they strike a
balance between between PR complexity and PR review overhead. PRs within the
optimal size (typical small, or medium sized PRs) mean:

Fast and predictable releases to production:
- Optimal size changes are more likely to be reviewed faster with fewer
  iterations.
- Similarity in low PR complexity drives similar review times.
Review quality is likely higher as complexity is lower:
- Bugs are more likely to be detected.
- Code inconsistencies are more likely to be detetcted.
Knowledge sharing is improved within the participants:
- Small portions can be assimilated better.
Better engineering practices are exercised:
- Solving big problems by dividing them in well contained, smaller problems.
- Exercising separation of concerns within the code changes.

What can I do to optimize my changes

Use the PullRequestQuantifier to quantify your PR accurately
- Create a context profile for your repo using the context generator
- Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
- Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
- Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
Change your engineering behaviors
- For PRs that fall outside of the desired spectrum, review the details and check if:
  - Your PR could be split in smaller, self-contained PRs instead
  - Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

One line was added: +1 -0
One line was deleted: +0 -1
One line was modified: +1 -1 (git diff doesn't know about modified, it will
interpret that line like one addition plus one deletion)
Change percentiles: Change characteristics (addition, deletion, modification)
of this PR in relation to all other PRs within the repository.

Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.

Performance improvements

5d32ad5

* Replaced a few "async Task" methods with simple "Task" methods to remove the async overhead * Made a few allocations lazy to avoid creating variable members when they aren't needed. * Removed the necessity to use a class based enumerator in two instances

pull-request-quantifier-deprecated bot added the Small label Oct 10, 2021

henning-krause mentioned this pull request Oct 10, 2021

Performance - OData response serialization is very expensive in CPU and memory #2182

Closed

Fixed failing unit tests

8807a9c

habbes reviewed Oct 25, 2021

View reviewed changes

src/Microsoft.OData.Core/InstanceAnnotationWriteTracker.cs Outdated Show resolved Hide resolved

Reverted changes to InstanceAnnotationWriteTracker to avoid merge issues

71bf1c0

pull-request-quantifier-deprecated bot added Medium and removed Small labels Oct 25, 2021

KenitoInc requested review from gathogojr, KenitoInc, xuzhg, chrisspre, mikepizzo and Sreejithpin October 28, 2021 12:02

odero reviewed Nov 8, 2021

View reviewed changes

src/Microsoft.OData.Core/InstanceAnnotationWriteTracker.cs Outdated Show resolved Hide resolved

Reverted changes in class InstanceAnnotationWriteTracker

5f836e7

pull-request-quantifier-deprecated bot added Small and removed Medium labels Nov 8, 2021

gathogojr reviewed Nov 10, 2021

View reviewed changes

src/Microsoft.OData.Core/Json/JsonValueUtilsAsync.cs Outdated Show resolved Hide resolved

gathogojr reviewed Nov 10, 2021

View reviewed changes

src/Microsoft.OData.Core/Json/JsonValueUtilsAsync.cs Outdated Show resolved Hide resolved

gathogojr reviewed Nov 10, 2021

View reviewed changes

Henning Krause added 2 commits November 10, 2021 18:07

Fixed issues from code review

9ddb010

Fixed issues from code review

9e32638

gathogojr approved these changes Nov 11, 2021

View reviewed changes

odero reviewed Nov 11, 2021

View reviewed changes

src/Microsoft.OData.Core/ODataAnnotatable.cs Outdated Show resolved Hide resolved

src/Microsoft.OData.Core/ODataAnnotatable.cs Outdated Show resolved Hide resolved

Resolved issue from code review

7c23162

Reverted WriteEscapedCharArrayAsync to be async again.

b3623b6

mikepizzo requested a review from corranrogue9 January 3, 2022 22:51

mikepizzo reviewed Jan 3, 2022

View reviewed changes

src/Microsoft.OData.Core/Json/JsonValueUtilsAsync.cs Outdated Show resolved Hide resolved

mikepizzo requested changes Jan 3, 2022

View reviewed changes

Changed usage of "var" to explicit type declaration

46aad16

mikepizzo approved these changes Jan 4, 2022

View reviewed changes

Merge branch 'master' into master

1c4d2ee

mikepizzo merged commit b092daf into OData:master Jan 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Performance improvements #2215

Performance improvements #2215

henning-krause commented Oct 10, 2021 •

edited

Loading

henning-krause commented Oct 25, 2021

odero commented Nov 8, 2021

henning-krause commented Nov 8, 2021

gathogojr commented Nov 8, 2021

mikepizzo commented Jan 3, 2022

mikepizzo left a comment

mikepizzo left a comment

pull-request-quantifier-deprecated bot commented Jan 4, 2022

What can I do to optimize my changes

How to interpret the change counts in git diff output

Performance improvements #2215

Performance improvements #2215

Conversation

henning-krause commented Oct 10, 2021 • edited Loading

Issues

Description

Checklist (Uncheck if it is not completed)

Additional work necessary

henning-krause commented Oct 25, 2021

odero commented Nov 8, 2021

henning-krause commented Nov 8, 2021

gathogojr commented Nov 8, 2021

mikepizzo commented Jan 3, 2022

mikepizzo left a comment

Choose a reason for hiding this comment

mikepizzo left a comment

Choose a reason for hiding this comment

pull-request-quantifier-deprecated bot commented Jan 4, 2022

What can I do to optimize my changes

How to interpret the change counts in git diff output

henning-krause commented Oct 10, 2021 •

edited

Loading