Skip to content

*: replace some usages of global rand#155797

Merged
craig[bot] merged 2 commits intocockroachdb:masterfrom
yuzefovich:global-rand
Oct 22, 2025
Merged

*: replace some usages of global rand#155797
craig[bot] merged 2 commits intocockroachdb:masterfrom
yuzefovich:global-rand

Conversation

@yuzefovich
Copy link
Copy Markdown
Member

@yuzefovich yuzefovich commented Oct 21, 2025

sem/builtins: use RNG from eval context for 'st_generatepoints'

*: replace some usages of global rand

When we use rand.* methods like math/rand.Intn, under the hood we hit
the global rand source, which is protected by a mutex, so we could be subject
to contention on that lock. This commit updates some of such usages in
favor of object-specific rand source to avoid that. Note that allocating
a separate rand source is not free (the allocation itself is about 5KiB
in size), so we modify spots where the lifecycle matches that of the
server or when the affected code is heavy already / not on the hot path
(also when it's clear that the access is from within a single
goroutine).

Epic: None
Release note: None

@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

When we use `rand.*` methods like `math/rand.Intn`, under the hood we hit
the global rand source, which is protected by a mutex, so we could be subject
to contention on that lock. This commit updates some of such usages in
favor of object-specific rand source to avoid that. Note that allocating
a separate rand source is not free (the allocation itself is about 5KiB
in size), so we modify spots where the lifecycle matches that of the
server or when the affected code is heavy already / not on the hot path
(also when it's clear that the access is from within a single
goroutine).

Release note: None
@blathers-crl
Copy link
Copy Markdown

blathers-crl bot commented Oct 22, 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.

@yuzefovich yuzefovich marked this pull request as ready for review October 22, 2025 05:03
@yuzefovich yuzefovich requested review from a team as code owners October 22, 2025 05:04
@yuzefovich yuzefovich requested review from a team, kyle-a-wong and michae2 and removed request for a team and kyle-a-wong October 22, 2025 05:04
Copy link
Copy Markdown
Collaborator

@michae2 michae2 left a comment

Choose a reason for hiding this comment

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

:lgtm: Nice!

@michae2 reviewed 1 of 1 files at r1, 11 of 11 files at r2, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @yuzefovich)

@yuzefovich
Copy link
Copy Markdown
Member Author

TFTR!

bors r+

craig bot pushed a commit that referenced this pull request Oct 22, 2025
155616: rangefeed: add assertion that OnError is called on disconnected registration r=stevendanna a=stevendanna

Epic: none
Release note: None

155797: *: replace some usages of global rand r=yuzefovich a=yuzefovich

**sem/builtins: use RNG from eval context for 'st_generatepoints'**

***: replace some usages of global rand**

When we use `rand.*` methods like `math/rand.Intn`, under the hood we hit
the global rand source, which is protected by a mutex, so we could be subject
to contention on that lock. This commit updates some of such usages in
favor of object-specific rand source to avoid that. Note that allocating
a separate rand source is not free (the allocation itself is about 5KiB
in size), so we modify spots where the lifecycle matches that of the
server or when the affected code is heavy already / not on the hot path
(also when it's clear that the access is from within a single
goroutine).

Epic: None
Release note: None

155861: kvserver: deflake TestPromoteNonVoterInAddVoter r=arulajmani a=arulajmani

This test could previously fail because a lease transfer could race with a split. Without the split, the effects of the span config (that the test asserts on) will not apply. We sidestep this problem by removing the need of a split entirely -- by tuning some cluster settings to ensure that every table is on its own range to begin with.

H/t to `@pav-kv` for the analysis on this one.

Closes #155828 
Closes #155747 
Closes #155316 
Closes #154773 
Closes #134383

Release note: None

Co-authored-by: Steven Danna <danna@cockroachlabs.com>
Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>
Co-authored-by: Arul Ajmani <arulajmani@gmail.com>
@craig
Copy link
Copy Markdown
Contributor

craig bot commented Oct 22, 2025

Build failed (retrying...):

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Oct 22, 2025

@craig craig bot merged commit b59054b into cockroachdb:master Oct 22, 2025
26 checks passed
@yuzefovich yuzefovich deleted the global-rand branch October 22, 2025 20:21
rishabh7m added a commit to crltest-issue-syncing/cockroach-fork that referenced this pull request Nov 20, 2025
rishabh7m added a commit to crltest-issue-syncing/cockroach-fork that referenced this pull request Nov 21, 2025
rishabh7m added a commit to crltest-issue-syncing/cockroach-fork that referenced this pull request Nov 21, 2025
rishabh7m added a commit to crltest-issue-syncing/cockroach-fork that referenced this pull request Dec 1, 2025
rishabh7m added a commit to crltest-issue-syncing/cockroach-fork that referenced this pull request Dec 10, 2025
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