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

sql: grow stacks for better performance (~3.00% cpu or more) #130663

Open
tbg opened this issue Sep 13, 2024 · 3 comments · May be fixed by #137743
Open

sql: grow stacks for better performance (~3.00% cpu or more) #130663

tbg opened this issue Sep 13, 2024 · 3 comments · May be fixed by #137743
Assignees
Labels
C-performance Perf of queries or internals. Solution not expected to change functional behavior. o-perf-efficiency Related to performance efficiency P-2 Issues/test failures with a fix SLA of 3 months T-sql-queries SQL Queries Team

Comments

@tbg
Copy link
Member

tbg commented Sep 13, 2024

Is your feature request related to a problem? Please describe.

I ran1 sysbenchs oltp_read_only against a small table distributed across an three-node n2-standard-4 roachprod GCE cluster, via:

sysbench --db-driver=pgsql --pgsql-host=127.0.0.1 --pgsql-port=26257
     --pgsql-user=roachprod --pgsql-password=cockroachdb --pgsql-db=sysbench \
     --report-interval=1 --time=3600 \
     --threads=96 --tables=1 --table_size=10000 --auto_inc=false oltp_read_only run

Under this workload (90+% average CPU utilization), we spend around 7%-10% of the time accounted for in CPU profiles growing stacks (the Go stack starts at 2kb).

image

Describe the solution you'd like

We already know that we often exceed the default Go stack size, and in a few places have worked around it by pre-growing stacks early, for example:

/pkg/rpc/context.go#L1671-L1678

// Unmarshal detects BatchRequests and calls growstack.Grow before calling
// through to the underlying codec.
func (c growStackCodec) Unmarshal(data []byte, v interface{}) error {
	if _, ok := v.(*kvpb.BatchRequest); ok {
		growstack.Grow()
	}
	return c.Codec.Unmarshal(data, v)
}

and

/pkg/sql/internal.go#L250-L254

			// TODO(yuzefovich): benchmark whether we should be growing the
			// stack size unconditionally.
			if growStackSize {
				growstack.Grow()
			}

A uniting theme between the different "flames" in the screenshot above is that they are in DistSQL processing, and likely all bypass these two calls to growstack.Grow.

Addressing this would likely be rewarded with a noticeable performance improvement on this and other DistSQL-heavy workloads.

Describe alternatives you've considered

Additional context

One interesting alternative, at least for a prototype/exploration, is changing our go fork's default stack size.

Jira issue: CRDB-42173

Footnotes

  1. see internal doc for full steps.

@tbg tbg added the C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) label Sep 13, 2024
@tbg tbg added this to SQL Queries Sep 13, 2024
@github-project-automation github-project-automation bot moved this to Triage in SQL Queries Sep 13, 2024
@mgartner mgartner added C-performance Perf of queries or internals. Solution not expected to change functional behavior. and removed C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) labels Sep 17, 2024
@mgartner mgartner moved this from Triage to Backlog in SQL Queries Sep 17, 2024
@tbg tbg added P-2 Issues/test failures with a fix SLA of 3 months o-perf-efficiency Related to performance efficiency T-sql-queries SQL Queries Team labels Oct 23, 2024
@tbg
Copy link
Member Author

tbg commented Oct 23, 2024

From https://github.com/cockroachlabs/perf-efficiency-team/pull/4

3%, so still quite sizeable.

image

@tbg tbg changed the title sql: grow stacks for better performance sql: grow stacks for better performance (~3% cpu or more) Oct 25, 2024
@tbg tbg changed the title sql: grow stacks for better performance (~3% cpu or more) sql: grow stacks for better performance (~3.00% cpu or more) Oct 25, 2024
@tbg
Copy link
Member Author

tbg commented Nov 28, 2024

Another one. This has distsql turned off, but looks generally similar.

profile.pb.gz

image

@tbg
Copy link
Member Author

tbg commented Dec 18, 2024

on tpcc-nowait, 1000 warehouses. Note that the bottom stack frame (at the top) is always Stopper.RunAsyncTask, so we could plop a growstack call in there.

image

@ajstorm ajstorm assigned ajstorm and tbg and unassigned ajstorm Jan 13, 2025
@tbg tbg linked a pull request Jan 14, 2025 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-performance Perf of queries or internals. Solution not expected to change functional behavior. o-perf-efficiency Related to performance efficiency P-2 Issues/test failures with a fix SLA of 3 months T-sql-queries SQL Queries Team
Projects
Status: Backlog
Development

Successfully merging a pull request may close this issue.

3 participants