Skip to content
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: Fix OM hierarchy to use strings for relative paths instead of URI #1617

Merged
merged 17 commits into from
Jul 2, 2020

Conversation

kirankumarkolli
Copy link
Member

@kirankumarkolli kirankumarkolli commented Jun 13, 2020

1.5% CPU loss observed through profiles.
Though URI is created Direct takes plain string and its a wasted CPU cycles.

Microbenchmark shows around 5-6% gain.
For real workload it might be close to 1% gain.

Summary of benchmark from 4core Linux .Net core 3.1.
image

Method Type Mean Error StdDev Median Q3 P80 P85 P90 P95 P100 Gen 0 Gen 1 Gen 2 Allocated
ReadItemExists Stream 483.8 us 55.85 us 281.0 us 355.9 us 444.8 us 470.5 us 802.0 us 925.8 us 1,168.8 us 1,536.5 us - - - 34.48 KB
ReadItemExists Stream 496.5 us 57.19 us 288.3 us 345.4 us 637.8 us 756.0 us 811.4 us 1,003.9 us 1,162.3 us 1,448.7 us - - - 25.27 KB
ReadItemExists Stream 467.2 us 49.58 us 250.8 us 358.4 us 451.0 us 464.9 us 755.6 us 953.9 us 1,058.5 us 1,478.0 us - - - 34.07 KB
ReadItemExists Stream 531.1 us 69.64 us 352.3 us 338.4 us 765.4 us 842.5 us 1,028.9 us 1,102.6 us 1,336.1 us 1,617.2 us - - - 34.07 KB
ReadItemExists Stream 465.6 us 58.09 us 285.8 us 342.0 us 372.8 us 407.4 us 801.0 us 873.1 us 1,181.5 us 1,557.3 us - - - 34.07 KB
Method Type Mean Error StdDev Median Q3 P80 P85 P90 P95 P100 Gen 0 Gen 1 Gen 2 Allocated
ReadItemExists Stream 506.5 us 65.99 us 331.4 us 345.3 us 428.6 us 510.5 us 987.6 us 1,059.2 us 1,251.2 us 1,969.3 us - - - 34.13 KB
ReadItemExists Stream 461.5 us 53.62 us 269.8 us 337.4 us 425.8 us 445.4 us 803.2 us 899.6 us 1,068.4 us 1,664.0 us - - - 34.13 KB
ReadItemExists Stream 527.3 us 62.26 us 315.0 us 349.5 us 769.4 us 847.3 us 936.4 us 1,056.5 us 1,188.2 us 1,548.3 us - - - 34.13 KB
ReadItemExists Stream 534.8 us 59.21 us 299.6 us 409.4 us 751.0 us 809.6 us 943.7 us 1,015.8 us 1,142.9 us 1,590.6 us - - - 34.13 KB
ReadItemExists Stream 568.2 us 68.28 us 347.9 us 376.7 us 808.5 us 933.9 us 1,032.7 us 1,090.5 us 1,313.3 us 1,820.4 us - - - 34.13 KB

@kirankumarkolli kirankumarkolli changed the title Performance: Fix hierarchy to use strings for relative paths instead of URI Performance: Fix OM hierarchy to use strings for relative paths instead of URI Jun 13, 2020
ealsur
ealsur previously approved these changes Jun 13, 2020
@kirankumarkolli kirankumarkolli dismissed stale reviews from j82w and ealsur via 2136e20 June 15, 2020 15:38
# Conflicts:
#	Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.Tests/FeedRange/ReadFeedTokenIteratorCoreTests.cs
# Conflicts:
#	Microsoft.Azure.Cosmos/src/Handler/RequestInvokerHandler.cs
j82w
j82w previously approved these changes Jun 18, 2020
@kirankumarkolli
Copy link
Member Author

@sboshra , @bchong95 please review the PR

sboshra
sboshra previously approved these changes Jun 24, 2020
Copy link
Contributor

@sboshra sboshra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

# Conflicts:
#	Microsoft.Azure.Cosmos/src/Resource/ClientContextCore.cs
#	Microsoft.Azure.Cosmos/src/Resource/Conflict/ConflictsCore.cs
#	Microsoft.Azure.Cosmos/src/Resource/Container/ContainerCore.cs
#	Microsoft.Azure.Cosmos/src/Resource/Permission/PermissionCore.cs
#	Microsoft.Azure.Cosmos/src/Resource/QueryResponses/ChangeFeedIteratorCore.cs
#	Microsoft.Azure.Cosmos/src/Resource/QueryResponses/ChangeFeedPartitionKeyResultSetIteratorCore.cs
#	Microsoft.Azure.Cosmos/src/Resource/Scripts/ScriptsCore.cs
#	Microsoft.Azure.Cosmos/src/Resource/User/UserCore.cs
@kirankumarkolli kirankumarkolli dismissed stale reviews from sboshra and j82w via dc1d5ab July 1, 2020 13:18
j82w
j82w previously approved these changes Jul 1, 2020
@kirankumarkolli kirankumarkolli merged commit 835077b into master Jul 2, 2020
@kirankumarkolli kirankumarkolli deleted the users/kirankk/exclude_uri_relative branch July 2, 2020 14:18
kirankumarkolli added a commit that referenced this pull request Jul 11, 2020
…ad of URI (#1617)

* Excluding unnecessary Uri from main line paths

* Fixing the bug

* Including missed optimization

* Some more related refactoring

* Unit test fixes

* Contract just to exclude compiler generated attribute

* Building emulator projects

* Moving all exiting URI references also to string

* Fixing build break

* Marking RequestUriString readonly

Co-authored-by: j82w <[email protected]>
@ghost
Copy link

ghost commented Dec 15, 2021

Closing due to in-activity, pease feel free to re-open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants