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: initialize virtual table descriptors concurrently; skip some vtable validations #142012

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

rafiss
Copy link
Collaborator

@rafiss rafiss commented Feb 25, 2025

See individual commits.

Epic: None

@rafiss rafiss added the do-not-merge bors won't merge a PR with this label. label Feb 25, 2025
Copy link

blathers-crl bot commented Feb 25, 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

@rafiss rafiss force-pushed the concurrent-init-vtable branch from c803c31 to 8b3928b Compare February 26, 2025 15:24
@rafiss rafiss changed the title sql: initialize virtual table descriptors concurrently sql: initialize virtual table descriptors concurrently; skip some vtable validations Feb 26, 2025
@rafiss rafiss force-pushed the concurrent-init-vtable branch from 8b3928b to 3a75994 Compare February 26, 2025 15:51
This helps improve TestServer startup time.

While working on this, I experimented with adding a concurrency limit,
which didn't turn out to matter. However, I still kept the addition of
SetLimit to the ctxgroup API, since it is generally useful.

Release note: None
This patch skips an expensive validation step when running in a unit
test. The virtual tables are all initialized on startup, and validating
all of them at once leads to many heap allocations that cause TestServer
startup to take longer.

Release note: None
@rafiss rafiss force-pushed the concurrent-init-vtable branch from 3a75994 to 959ef1f Compare February 26, 2025 18:58
@rafiss rafiss removed the do-not-merge bors won't merge a PR with this label. label Feb 26, 2025
@rafiss rafiss marked this pull request as ready for review February 26, 2025 18:59
@rafiss rafiss requested a review from a team as a code owner February 26, 2025 18:59
Copy link
Collaborator

@fqazi fqazi left a comment

Choose a reason for hiding this comment

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

Nice!
:lgtm:

Reviewed 1 of 1 files at r1, 2 of 2 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @rafiss)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants