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

quotapool,kvserver: extend quotapool.RateLimit, rate limit queue addition in SystemConfigUpdate #53605

Conversation

ajwerner
Copy link
Contributor

See individual commits.

This PR is a fancier implementation of #53603 that permits bursting and makes
setting a relatively low rate feel less risky in the face of a small burst of
changes.

Release justification: low risk, high benefit changes to existing functionality

Release note (performance improvement): Limited the frequency of an expensive
operation due to schema changes making workload which perform schema changes
at a high rate less resource intensive.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@ajwerner ajwerner force-pushed the ajwerner/dont-add-to-queues-on-every-single-system-config-update branch from 6cfdfc4 to 1898ec9 Compare August 28, 2020 22:25
Release justification: non-production code changes
Release note: None
Release justification: low risk, high benefit changes to existing functionality
Release note: None
Release justification: non-production code changes

Release note: None
…replicas

This has a shockingly profound impact on the runtime of ORM tests.

This PR is a fancier implementation of cockroachdb#53603 that permits bursting and makes
setting a relatively low rate feel less risky in the face of a small burst of
changes.

Release justification: low risk, high benefit changes to existing functionality

Release note (performance improvement): Limited the frequency of an expensive
operation due to schema changes making workload which perform schema changes
at a high rate less resource intensive.
@ajwerner ajwerner force-pushed the ajwerner/dont-add-to-queues-on-every-single-system-config-update branch from 1898ec9 to ab3f53d Compare August 31, 2020 02:43
@ajwerner ajwerner requested a review from tbg August 31, 2020 02:44
Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

I just reviewed this and I feel rather comfortable by the functional correctness of the thing (i.e. I can't see it blocking unexpectedly)

Can you speak a bit more quantitatively about what motivated this change and the visible benefits?

Reviewed 2 of 2 files at r1, 2 of 2 files at r2.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @knz and @tbg)

Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r4.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @knz and @tbg)

@ajwerner
Copy link
Contributor Author

Prior to this change, over 70% of the CPU utilization of the typeorm roachtest was underneath those calls. The change decreases the runtime of the typeorm by 30% (after applying #53589, have not measured the impact of this change against master but I'd suspect it to be even larger as that PR reduces the amount of work done to run jobs). I'm rerunning on top of master and against master to compare those runtime and CPU differences.

image

Before:

--- PASS: typeorm (1444.19s)
--- PASS: typeorm (1463.58s)
--- PASS: typeorm (1530.58s)

After

--- PASS: typeorm (1094.87s)
--- PASS: typeorm (1094.73s)
--- PASS: typeorm (1084.94s)

@ajwerner
Copy link
Contributor Author

ajwerner commented Aug 31, 2020

According to a 30s profile from before and after, this change reduces the CPU utilization due to the calls to adding to the queue from 69.7% to 4.9% and reduces the CPU utilization overall dramatically. I'll post runtime differences when they finish. See the below charts:

Before:
image

After:
image

@knz
Copy link
Contributor

knz commented Aug 31, 2020

this looks gorgeous!

I'm LGTM on this

@ajwerner
Copy link
Contributor Author

TFTR!

bors r=knz

typeorm runtime:

Before:

--- PASS: typeorm (1851.56s)
--- PASS: typeorm (1866.40s)
--- PASS: typeorm (1927.17s)

After:

--- PASS: typeorm (1599.05s)
--- PASS: typeorm (1632.52s)
--- PASS: typeorm (1640.89s)

@craig
Copy link
Contributor

craig bot commented Aug 31, 2020

Build succeeded:

@craig craig bot merged commit 410616f into cockroachdb:master Aug 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
KV 20.2
  
Awaiting triage
Development

Successfully merging this pull request may close these issues.

None yet

3 participants