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

split: init replica lb splitter with global rand #94754

Merged
merged 1 commit into from
Jan 5, 2023

Conversation

kvoli
Copy link
Collaborator

@kvoli kvoli commented Jan 5, 2023

In #93838 we started initializing a new seeded rand for use in the load
based splitter's reservoir sampling algorithm. Previously, the splitter
was initialized using the global rand.

rand.Source heap allocates on init approximately 4kb. When initialized
per-replica, this is problematic as nodes scale to large replica counts.

This patch replaces initializing a new random source per-replica with
using the global rand instead.

This removes the heap allocation. This is shown below running
kv/splits/nodes=3/quiesce=true (not at identical replica counts in the test,
the proportions are the important part).

before:

image

after:

image

resolves: #94752
resolves: #94737

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@kvoli kvoli self-assigned this Jan 5, 2023
@kvoli kvoli marked this pull request as ready for review January 5, 2023 16:07
@kvoli kvoli requested a review from a team as a code owner January 5, 2023 16:07
@kvoli kvoli force-pushed the 230105.randsource-mem branch from 03b2d9a to d64bc43 Compare January 5, 2023 16:08
Copy link
Contributor

@erikgrinaker erikgrinaker left a comment

Choose a reason for hiding this comment

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

I'm assuming the default source is seeded here?

cockroach/pkg/cli/cli.go

Lines 54 to 55 in 9351196

// Seed the math/rand RNG from crypto/rand.
rand.Seed(randutil.NewPseudoSeed())

In cockroachdb#93838 we started initializing a new seeded rand for use in the load
based splitter's reservoir sampling algorithm. Previously, the splitter
was initialized using the global rand.

`rand.Source` heap allocates on init approximately 4kb. When initialized
per-replica, this is problematic as nodes scale to large replica counts.

This patch replaces initializing a new random source per-replica with
using the global rand instead.

resolves: cockroachdb#94752
resolves: cockroachdb#94737

Release note: None
@kvoli kvoli force-pushed the 230105.randsource-mem branch from d64bc43 to 891e7a0 Compare January 5, 2023 16:52
@kvoli
Copy link
Collaborator Author

kvoli commented Jan 5, 2023

I'm assuming the default source is seeded here?

Yeah - perhaps a misleading title/commit msg. I've updated it to be more specific.

@kvoli kvoli changed the title split: use global rand in place of seeded split: init replica lb splitter with global rand Jan 5, 2023
@kvoli
Copy link
Collaborator Author

kvoli commented Jan 5, 2023

bors r=erikgrinaker

TYFTR

@craig
Copy link
Contributor

craig bot commented Jan 5, 2023

Build succeeded:

@craig craig bot merged commit e7713f1 into cockroachdb:master Jan 5, 2023
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.

split: replace seeded random source roachtest: kv/splits/nodes=3/quiesce=true failed
3 participants