Skip to content

Conversation

@yuzefovich
Copy link
Member

@yuzefovich yuzefovich commented Nov 6, 2025

This commit makes it so that the main goroutine of Outbox and the vectorized hash router goroutine now start out with the larger stack.

Addresses: #130663
Epic: None
Release note: None

@blathers-crl
Copy link

blathers-crl bot commented Nov 6, 2025

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@yuzefovich yuzefovich added X-perf-check Microbenchmarks CI: Added to a PR if a performance regression is detected and should be checked and removed X-perf-check Microbenchmarks CI: Added to a PR if a performance regression is detected and should be checked labels Nov 6, 2025
This commit makes it so that the main goroutine of Outbox and the
vectorized hash router goroutine now start out with the larger stack.

Release note: None
@yuzefovich yuzefovich requested a review from mgartner November 7, 2025 00:47
@yuzefovich yuzefovich marked this pull request as ready for review November 7, 2025 00:47
@yuzefovich yuzefovich requested a review from a team as a code owner November 7, 2025 00:47
@yuzefovich
Copy link
Member Author

yuzefovich commented Nov 7, 2025

These goroutines previously showed up noticeably (like here), but not as much anymore in sysbench (probably because we improved distsql heuristics). I tried to coerce the sysbench microbenchmark to show any impact (using distsql=on and creating the tables on n2 while running queries from n1), but there was no change. Still, it makes sense that we'd need to grow the stack for these goroutines - we can do lots of execution processing inside, and since we're not using the stopper to start them, we don't benefit from 1aab60e. So this change seems sound to me, but wouldn't move the needle much.

(Initially I also adjusted growStackCodec to handle SetupFlowRequest in addition to kvpb.BatchRequest but decided to back out that change.)

Copy link
Collaborator

@mgartner mgartner left a comment

Choose a reason for hiding this comment

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

Thanks for following up on this!

@yuzefovich
Copy link
Member Author

TFTR!

bors r+

@craig
Copy link
Contributor

craig bot commented Nov 8, 2025

@craig craig bot merged commit 8ca1920 into cockroachdb:master Nov 8, 2025
26 checks passed
@yuzefovich yuzefovich deleted the distsql-growstack branch November 8, 2025 00:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants