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

[vtgate] Add flag to pool connection read buffers #11167

Merged

Conversation

brirams
Copy link
Collaborator

@brirams brirams commented Sep 1, 2022

Description

We(Slack) found that when a vtgate is under heavy connection load, tail latencies of connect times
tend to spike into the tens of milliseconds. A contributing factor is GC slowdown that is
exacerbated by the repeated allocation of enw 16k read buffers for each incoming connection. This PR
adds a flag to the vtgate mysql server to enable pooling those read buffers and re-using them across
requests.

@henryr found that under a synthetic load, we were able to get connection times down by 55% at the
p999 and 40% at the p99:

Screen Shot 2022-08-16 at 10 37 16 AM

We've also run this against our loadtest environment and found, aside from less GC pauses, that our
upstream clients saw a 20% reduction in connection failures, with a 5-8% drop in connect latencies
across the board(p50 -> p999), and no perceived degradation in query latency.

This type of change has been attempted in the past
and was closed out but since there has since been work to buffer connection writes, figured it made
sense to revisit that.

Related Issue(s)

#4186

Checklist

  • "Backport me!" label has been added if this change should be backported
  • Tests were added or are not required
  • Documentation was added or is not required

Deployment Notes

n/a

@vitess-bot
Copy link
Contributor

vitess-bot bot commented Sep 1, 2022

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

  • Ensure that the Pull Request has a descriptive title.
  • If this is a change that users need to know about, please apply the release notes (needs details) label so that merging is blocked unless the summary release notes document is included.
  • If a new flag is being introduced, review whether it is really needed. The flag names should be clear and intuitive (as far as possible), and the flag's help should be descriptive. Additionally, flag names should use dashes (-) as word separators rather than underscores (_).
  • If a workflow is added or modified, each items in Jobs should be named in order to mark it as required. If the workflow should be required, the GitHub Admin should be notified.

Bug fixes

  • There should be at least one unit or end-to-end test.
  • The Pull Request description should either include a link to an issue that describes the bug OR an actual description of the bug and how to reproduce, along with a description of the fix.

Non-trivial changes

  • There should be some code comments as to why things are implemented the way they are.

New/Existing features

  • Should be documented, either by modifying the existing documentation or creating new documentation.
  • New features should have a link to a feature request issue or an RFC that documents the use cases, corner cases and test cases.

Backward compatibility

  • Protobuf changes should be wire-compatible.
  • Changes to _vt tables and RPCs need to be backward compatible.
  • vtctl command output order should be stable and awk-able.

@brirams brirams changed the title [vtgate] Add ability flag to pool connection read buffers [vtgate] Add flag to pool connection read buffers Sep 3, 2022
@deepthi deepthi requested a review from vmg September 8, 2022 21:58
@deepthi deepthi added Component: Query Serving Type: Enhancement Logical improvement (somewhere between a bug and feature) Type: Performance labels Sep 8, 2022
Copy link
Member

@harshit-gangal harshit-gangal left a comment

Choose a reason for hiding this comment

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

can we run the benchmarks from here go/mysql/query_benchmark_test.go if it gives some additional insights?

go/mysql/conn.go Outdated Show resolved Hide resolved
@brirams
Copy link
Collaborator Author

brirams commented Sep 20, 2022

Benchmark output from this branch(still need to compare with main but figured this was good to have for now):

 ~/n/d/vitess/go/mysql $ go test -v -bench=.
<elided>
goos: darwin
goarch: amd64
pkg: vitess.io/vitess/go/mysql
cpu: Intel(R) Core(TM) i9-9980HK CPU @ 2.40GHz
BenchmarkParallelShortQueries
BenchmarkParallelShortQueries-16     	       1	1428472710 ns/op	   0.00 MB/s
BenchmarkParallelMediumQueries
BenchmarkParallelMediumQueries-16    	       1	1482719290 ns/op	   0.01 MB/s
BenchmarkParallelRandomQueries
BenchmarkParallelRandomQueries-16    	       1	1467065363 ns/op
PASS

…stener and connection

Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>
Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>
Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>
@brirams brirams force-pushed the brirams_henryr_pool_read_buffers branch from 878a51f to 59208c5 Compare September 21, 2022 15:14
@brirams
Copy link
Collaborator Author

brirams commented Sep 21, 2022

results off of the main branch:

~/n/d/vitess/go/mysql $ go test -v -bench=.
<elided>
goos: darwin
goarch: amd64
pkg: vitess.io/vitess/go/mysql
cpu: Intel(R) Core(TM) i9-9980HK CPU @ 2.40GHz
BenchmarkParallelShortQueries
BenchmarkParallelShortQueries-16     	       1	1588925544 ns/op	   0.00 MB/s
BenchmarkParallelMediumQueries
BenchmarkParallelMediumQueries-16    	       1	1576328375 ns/op	   0.01 MB/s
BenchmarkParallelRandomQueries
BenchmarkParallelRandomQueries-16    	       1	1627565065 ns/op
PASS

Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>
@brirams
Copy link
Collaborator Author

brirams commented Sep 21, 2022

modified the benchmark to run with the read buffer pooling enable and disabled so we can compare more effectively:

~/n/d/vitess/go/mysql $ go test -v -bench=.
<elided>
goos: darwin
goarch: amd64
pkg: vitess.io/vitess/go/mysql
cpu: Intel(R) Core(TM) i9-9980HK CPU @ 2.40GHz
BenchmarkParallelShortQueries
BenchmarkParallelShortQueries-16                          	       1	1820589019 ns/op	   0.00 MB/s
BenchmarkParallelMediumQueries
BenchmarkParallelMediumQueries-16                         	       1	1798463863 ns/op	   0.01 MB/s
BenchmarkParallelRandomQueries
BenchmarkParallelRandomQueries-16                         	       1	1712101024 ns/op
BenchmarkParallelShortQueriesWithReadBufferPooling
BenchmarkParallelShortQueriesWithReadBufferPooling-16     	       1	1687282698 ns/op	   0.00 MB/s
BenchmarkParallelMediumQueriesWithReadBufferPooling
BenchmarkParallelMediumQueriesWithReadBufferPooling-16    	       1	1740339458 ns/op	   0.01 MB/s
BenchmarkParallelRandomQueriesWithReadBufferPooling
BenchmarkParallelRandomQueriesWithReadBufferPooling-16    	       1	1702365597 ns/op
PASS

@harshit-gangal harshit-gangal marked this pull request as ready for review September 22, 2022 12:58
Copy link
Member

@harshit-gangal harshit-gangal left a comment

Choose a reason for hiding this comment

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

changes looks good, just one flag naming convention change.

@harshit-gangal harshit-gangal added the release notes (needs details) This PR needs to be listed in the release notes in a dedicated section (deprecation notice, etc...) label Sep 22, 2022
@harshit-gangal
Copy link
Member

Would need to add in release notes for the new flag and what it means over https://github.com/vitessio/vitess/blob/main/doc/releasenotes/15_0_0_summary.md

Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>
Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>
Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>
Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>
@harshit-gangal harshit-gangal removed the release notes (needs details) This PR needs to be listed in the release notes in a dedicated section (deprecation notice, etc...) label Sep 26, 2022
@harshit-gangal harshit-gangal merged commit 6bee2fe into vitessio:main Sep 26, 2022
brirams added a commit to slackhq/vitess that referenced this pull request Sep 27, 2022
Signed-off-by: Brian Ramos <brirams@users.noreply.github.com>
brirams added a commit to slackhq/vitess that referenced this pull request Sep 29, 2022
As the title suggests, this is a backport of a change that was made in upstream for the v15 release.
tanjinx added a commit to slackhq/vitess that referenced this pull request Dec 8, 2022
tanjinx added a commit to slackhq/vitess that referenced this pull request Dec 9, 2022
tanjinx added a commit to slackhq/vitess that referenced this pull request May 4, 2023
@timvaillancourt timvaillancourt deleted the brirams_henryr_pool_read_buffers branch May 4, 2023 16:36
tanjinx added a commit to slackhq/vitess that referenced this pull request May 4, 2023
* Backport vitessio#11167-vtgate buffer fix

* update help message

* fix help message
@mattlord mattlord mentioned this pull request Jun 26, 2023
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Query Serving Type: Enhancement Logical improvement (somewhere between a bug and feature) Type: Performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants