-
Notifications
You must be signed in to change notification settings - Fork 108
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
Improve performance of get account by ID on Citus #5351
Conversation
Signed-off-by: Jesse Nelson <jesse@swirldslabs.com>
Signed-off-by: Jesse Nelson <jesse@swirldslabs.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5351 +/- ##
============================================
- Coverage 92.60% 92.60% -0.01%
+ Complexity 3094 3093 -1
============================================
Files 565 565
Lines 18300 18302 +2
Branches 1963 1964 +1
============================================
+ Hits 16947 16948 +1
Misses 982 982
- Partials 371 372 +1 ☔ View full report in Codecov by Sentry. |
const {query: entityQuery, params: entityParams} = getAccountQuery( | ||
{query: 'e.id = ?', params: accountIdParams}, | ||
{query: 'e.id = ? and (es.id = ? OR es.id IS NULL)', params: accountIdParams}, |
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.
Need k6 report.md results from before and after. Also need to ensure no regression on v1.
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 ensured only minor regression on v1 (-20 ms average difference, -50 RPS). I ran for v2 and there was a decrease of about 4s on the first run and negligible difference on a second run. I will drop some screen shots here
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.
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.
Not sure what improvement you're seeing. Looks like 86 rps in both. But again, that output is not accurate. We need the report.md
output and in particular the pass RPS.
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.
When I ran things for the first pass, the before was 21.2 s average and the after was 17.3 s average. The second pass, is what these screenshots are and yes they are pretty much identical. That was actually to be expected though. This runs in parallel with the get account related transactions query which is less performant than the entity query.
These results are accurate when just running a single test correct? All that was ran was the accountId.js test. Should I be running the whole suite? The change here is only going to effect the single endpoint.
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.
Perhaps manually disable transactions query and re-run single test. But I want the output of report.md, not k6 stdout.
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.
That is a useful test to run and will do so. However, report.md can't be generated when running a single test.
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.
V1 Before
Scenario | URL | VUS | Pass% | RPS | Pass RPS | Avg. Req Duration | Comment |
---|---|---|---|---|---|---|---|
accounts | /accounts | 1500 | 100.00 | 605.29/s | 605.29/s | 1062.93ms | |
accountsBalanceFalse | /accounts?balance=false | 1500 | 100.00 | 1708.71/s | 1708.71/s | 431.99ms | |
accountsBalanceFalsePubkey | /accounts?balance=false&account.publickey={publicKey} | 1500 | 100.00 | 5720.93/s | 5720.93/s | 167.64ms | |
accountsBalanceGt0 | /accounts?account.balance=gt:0 | 1500 | 100.00 | 1690.43/s | 1690.43/s | 425.52ms | |
accountsBalanceGt0Pubkey | /accounts?account.balance=gt:0&account.publickey={publicKey} | 1500 | 100.00 | 5461.20/s | 5461.20/s | 190.01ms | |
accountsBalanceNe | /accounts?account.balance=ne:{balance}&order=desc | 1500 | 99.99 | 2154.36/s | 2154.14/s | 252.22ms | |
accountsId | /accounts/{accountId} | 1500 | 100.00 | 5615.04/s | 5615.04/s | 155.31ms | |
accountsIdNe | /accounts?account.id=ne:{accountId}&order=desc | 1500 | 100.00 | 2165.72/s | 2165.72/s | 250.63ms |
V1 After
Scenario | URL | VUS | Pass% | RPS | Pass RPS | Avg. Req Duration | Comment |
---|---|---|---|---|---|---|---|
accounts | /accounts | 1500 | 100.00 | 601.91/s | 601.91/s | 1026.58ms | |
accountsBalanceFalse | /accounts?balance=false | 1500 | 100.00 | 1693.33/s | 1693.33/s | 430.24ms | |
accountsBalanceFalsePubkey | /accounts?balance=false&account.publickey={publicKey} | 1500 | 100.00 | 5525.86/s | 5525.86/s | 165.50ms | |
accountsBalanceGt0 | /accounts?account.balance=gt:0 | 1500 | 100.00 | 1693.18/s | 1693.18/s | 430.18ms | |
accountsBalanceGt0Pubkey | /accounts?account.balance=gt:0&account.publickey={publicKey} | 1500 | 100.00 | 5619.96/s | 5619.96/s | 191.93ms | |
accountsBalanceNe | /accounts?account.balance=ne:{balance}&order=desc | 1500 | 100.00 | 2183.65/s | 2183.65/s | 250.21ms | |
accountsId | /accounts/{accountId} | 1500 | 100.00 | 5649.88/s | 5649.88/s | 154.13ms | |
accountsIdNe | /accounts?account.id=ne:{accountId}&order=desc | 1500 | 100.00 | 2176.97/s | 2176.97/s | 251.57ms |
V2 Before
Scenario | URL | VUS | Pass% | RPS | Pass RPS | Avg. Req Duration | Comment |
---|---|---|---|---|---|---|---|
accounts | /accounts | 1500 | 69.35 | 87.45/s | 60.65/s | 15756.76ms | |
accountsBalanceFalse | /accounts?balance=false | 1500 | 96.09 | 131.17/s | 126.04/s | 11021.34ms | |
accountsBalanceFalsePubkey | /accounts?balance=false&account.publickey={publicKey} | 1500 | 98.40 | 145.35/s | 143.02/s | 10125.77ms | |
accountsBalanceGt0 | /accounts?account.balance=gt:0 | 1500 | 86.25 | 101.66/s | 87.68/s | 13828.54ms | |
accountsBalanceGt0Pubkey | /accounts?account.balance=gt:0&account.publickey={publicKey} | 1500 | 93.96 | 118.75/s | 111.58/s | 12097.88ms | |
accountsBalanceNe | /accounts?account.balance=ne:{balance}&order=desc | 1500 | 89.74 | 106.89/s | 95.92/s | 13280.40ms | |
accountsId | /accounts/{accountId} | 1500 | 99.94 | 231.74/s | 231.60/s | 6449.45ms | |
accountsIdNe | /accounts?account.id=ne:{accountId}&order=desc | 1500 | 89.28 | 107.84/s | 96.28/s | 13100.56ms |
V2 After
Scenario | URL | VUS | Pass% | RPS | Pass RPS | Avg. Req Duration | Comment |
---|---|---|---|---|---|---|---|
accounts | /accounts | 1500 | 69.79 | 87.03/s | 60.74/s | 15901.94ms | |
accountsBalanceFalse | /accounts?balance=false | 1500 | 96.89 | 134.53/s | 130.35/s | 10820.26ms | |
accountsBalanceFalsePubkey | /accounts?balance=false&account.publickey={publicKey} | 1500 | 98.59 | 147.85/s | 145.77/s | 9966.11ms | |
accountsBalanceGt0 | /accounts?account.balance=gt:0 | 1500 | 86.71 | 102.03/s | 88.47/s | 13801.66ms | |
accountsBalanceGt0Pubkey | /accounts?account.balance=gt:0&account.publickey={publicKey} | 1500 | 94.00 | 120.14/s | 112.93/s | 11943.26ms | |
accountsBalanceNe | /accounts?account.balance=ne:{balance}&order=desc | 1500 | 89.61 | 108.89/s | 97.58/s | 12978.47ms | |
accountsId | /accounts/{accountId} | 1500 | 100.00 | 2784.57/s | 2784.57/s | 501.56ms | |
accountsIdNe | /accounts?account.id=ne:{accountId}&order=desc | 1500 | 89.82 | 107.65/s | 96.69/s | 13152.19ms |
…228-Citus-optimize-get-account-by-id-query
Signed-off-by: Jesse Nelson <jesse@swirldslabs.com>
…et-account-by-id-query
Kudos, SonarCloud Quality Gate passed! |
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.
LGTM
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.
LGTM
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.
Looks good!
improve performance of account by id for v2 * filter entity_stake by single account id * filter token_balance by single account id Signed-off-by: Jesse Nelson <jesse@swirldslabs.com> Signed-off-by: Marc Kriguer <marc.kriguer@swirldslabs.com>
improve performance of account by id for v2 * filter entity_stake by single account id * filter token_balance by single account id Signed-off-by: Jesse Nelson <jesse@swirldslabs.com> Signed-off-by: Marc Kriguer <marc.kriguer@swirldslabs.com>
improve performance of account by id for v2 * filter entity_stake by single account id * filter token_balance by single account id Signed-off-by: Jesse Nelson <jesse@swirldslabs.com> Signed-off-by: Marc Kriguer <marc.kriguer@swirldslabs.com>
Description:
Modify get account by id query to filter on single account id
Related issue(s):
Fixes #5228
Notes for reviewer:
Checklist