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

Query: Fixes query performance regression and adds mocked query benchmark #2676

Merged
merged 6 commits into from
Sep 8, 2021

Conversation

j82w
Copy link
Contributor

@j82w j82w commented Aug 18, 2021

Pull Request Template

Description

  1. Fixes a query performance regression caused by doing 2 hash lookups on all the headers. This now only does the validation for debug mode.
  2. Adds a mocked query benchmark to the performance tests

Benchmarks result after optimization. Will add before and after benchmarks in future iteration.

BenchmarkDotNet=v0.12.0, OS=Windows 10.0.19043
Intel Core i7-9700 CPU 3.00GHz, 1 CPU, 8 logical and 8 physical cores
.NET Core SDK=5.0.400
[Host] : .NET Core 3.1.18 (CoreCLR 4.700.21.35901, CoreFX 4.700.21.36305), X64 RyuJIT
MediumRun : .NET Core 3.1.18 (CoreCLR 4.700.21.35901, CoreFX 4.700.21.36305), X64 RyuJIT

Job=MediumRun IterationCount=15 LaunchCount=2
WarmupCount=10

Method Type Mean Error StdDev StdErr Min Q1 Median Q3 Max P80 P85 P90 P95 P100 Op/s Gen 0 Gen 1 Gen 2 Allocated Completed Work Items Lock Contentions
ReadFeed Stream 596.8 us 81.68 us 119.7 us 22.23 us 428.0 us 485.9 us 555.7 us 685.5 us 843.9 us 689.7 us 732.1 us 758.6 us 810.2 us 843.9 us 1,675.6 - - - 41.74 KB 1.0000 -
QuerySinglePage Stream 6,118.2 us 267.26 us 391.7 us 72.74 us 5,737.2 us 5,796.3 us 5,991.9 us 6,400.3 us 7,220.7 us 6,479.0 us 6,510.9 us 6,614.1 us 6,786.9 us 7,220.7 us 163.4 - - - 981.54 KB 1.0000 -
ReadFeed OfT 3,592.5 us 165.32 us 247.4 us 45.18 us 3,299.5 us 3,401.5 us 3,530.8 us 3,697.6 us 4,093.9 us 3,824.4 us 3,977.0 us 3,997.0 us 4,035.8 us 4,093.9 us 278.4 - - - 547.02 KB 1.0000 -
QuerySinglePage OfT 8,449.5 us 1,009.36 us 1,479.5 us 274.74 us 5,943.0 us 7,358.1 us 8,812.9 us 9,226.9 us 12,680.0 us 9,237.4 us 9,330.9 us 9,563.8 us 9,710.2 us 12,680.0 us 118.3 - - - 2189.16 KB 1.0000 -
ReadFeed OfTCustom 3,529.0 us 110.96 us 162.6 us 30.20 us 3,268.8 us 3,402.4 us 3,508.7 us 3,629.3 us 4,029.8 us 3,637.8 us 3,665.5 us 3,701.5 us 3,756.7 us 4,029.8 us 283.4 - - - 547 KB 1.0000 -
QuerySinglePage OfTCustom 8,478.4 us 759.40 us 1,089.1 us 205.82 us 5,850.9 us 8,528.0 us 8,750.3 us 9,045.4 us 10,715.0 us 9,061.9 us 9,115.1 us 9,154.0 us 9,441.2 us 10,715.0 us 117.9 - - - 2189.16 KB 1.0000 -
ReadFeed OfTWi(...)tring [26] 3,729.1 us 164.13 us 235.4 us 44.49 us 3,406.1 us 3,481.4 us 3,728.2 us 3,835.4 us 4,157.3 us 3,913.0 us 4,070.0 us 4,086.3 us 4,112.3 us 4,157.3 us 268.2 - - - 584.62 KB 1.0000 -
QuerySinglePage OfTWi(...)tring [26] 9,047.6 us 1,425.71 us 2,133.9 us 389.60 us 6,137.9 us 6,736.9 us 9,257.1 us 9,787.9 us 15,396.7 us 9,874.2 us 10,347.2 us 10,577.8 us 12,535.4 us 15,396.7 us 110.5 - - - 2250.66 KB 1.0000 -
ReadFeed OfTWi(...)abled [29] 3,642.8 us 128.77 us 188.7 us 35.05 us 3,360.4 us 3,504.3 us 3,570.0 us 3,792.2 us 4,271.3 us 3,810.3 us 3,820.9 us 3,825.6 us 3,859.5 us 4,271.3 us 274.5 - - - 549.94 KB 1.0000 -
QuerySinglePage OfTWi(...)abled [29] 8,356.5 us 936.03 us 1,401.0 us 255.79 us 6,029.0 us 6,835.5 us 8,710.9 us 8,974.3 us 12,302.3 us 9,120.1 us 9,433.2 us 9,595.5 us 9,987.1 us 12,302.3 us 119.7 - - - 2192.49 KB 1.0000 -

Type of change

Please delete options that are not relevant.

  • [] Bug fix (non-breaking change which fixes an issue)
  • [] New feature (non-breaking change which adds functionality)
  • [] Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • [] This change requires a documentation update

Closing issues

To automatically close an issue: closes #2656

@kirankumarkolli
Copy link
Member

Is the header validation to cover for scenarios where ContinuationToken is not supported?
(RU, ActivityId are even applicable for point operation as well)

@j82w
Copy link
Contributor Author

j82w commented Aug 19, 2021

Is the header validation to cover for scenarios where ContinuationToken is not supported?
(RU, ActivityId are even applicable for point operation as well)

The header validation is make sure that you are not putting a known header value like request charge into the additional header dictionary. This might happen new headers are added to the base class, but should never happen in production.

@j82w j82w merged commit 9d56895 into master Sep 8, 2021
@j82w j82w deleted the users/jawilley/perf/query_page branch September 8, 2021 15:29
@j82w
Copy link
Contributor Author

j82w commented Sep 10, 2021

Benchmark results before:
"Top10PercentAverageRps": 4829,
"Top20PercentAverageRps": 4821,
"Top30PercentAverageRps": 4812,
"Top40PercentAverageRps": 4805,
"Top50PercentAverageRps": 4798,
"Top60PercentAverageRps": 4790,
"Top70PercentAverageRps": 4779,
"Top80PercentAverageRps": 4766,
"Top90PercentAverageRps": 4751,
"Top95PercentAverageRps": 4737,
"Top99PercentAverageRps": 4700,
"Top50PercentLatencyInMs": 2.4613,
"Top75PercentLatencyInMs": 3.1592,
"Top90PercentLatencyInMs": 6.6559,
"Top95PercentLatencyInMs": 10.9384,
"Top98PercentLatencyInMs": 17.196569999999973,
"Top99PercentLatencyInMs": 21.84066833333331,
"MaxLatencyInMs": 209.0017,
"AverageRps": 4669,

After:
"Top10PercentAverageRps": 4992,
"Top20PercentAverageRps": 4981,
"Top30PercentAverageRps": 4972,
"Top40PercentAverageRps": 4965,
"Top50PercentAverageRps": 4959,
"Top60PercentAverageRps": 4951,
"Top70PercentAverageRps": 4941,
"Top80PercentAverageRps": 4928,
"Top90PercentAverageRps": 4915,
"Top95PercentAverageRps": 4906,
"Top99PercentAverageRps": 4880,
"Top50PercentLatencyInMs": 2.3936,
"Top75PercentLatencyInMs": 3.018,
"Top90PercentLatencyInMs": 6.4072,
"Top95PercentLatencyInMs": 10.6983,
"Top98PercentLatencyInMs": 16.91974999999986,
"Top99PercentLatencyInMs": 21.50516833333331,
"MaxLatencyInMs": 200.9202,
"AverageRps": 4861,

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.

Query performance regression in 3.17.0+
3 participants