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

cli: change jemalloc defaults #139743

Merged
merged 1 commit into from
Jan 25, 2025

Conversation

RaduBerinde
Copy link
Member

@RaduBerinde RaduBerinde commented Jan 24, 2025

This commit changes the jemalloc configuration to reduce Cgo total
spikes. This configuration has shown to reduce spikes without any
noticeable impact to performance.

Epic: none
Release note: None

Full experiment results (note the multiple tabs): https://docs.google.com/spreadsheets/d/1Vs9hx0wytkySIvW3DRXI4MXC49b_9Bgkk_IPsoKFH6M/edit?gid=0#gid=0

This is a before/after of sysbench with 3 nodes and 800M rows:
image

@RaduBerinde RaduBerinde requested review from nicktrav, jbowens, sumeerbhola and a team January 24, 2025 15:57
@RaduBerinde RaduBerinde requested review from a team as code owners January 24, 2025 15:57
Copy link

blathers-crl bot commented Jan 24, 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

@RaduBerinde
Copy link
Member Author

Any thoughts on backporting to 25.1? I would be in favor, it seems like a row risk change.

Copy link
Collaborator

@sumeerbhola sumeerbhola left a comment

Choose a reason for hiding this comment

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

Great! I'm in favor of backporting.

:lgtm:

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @jbowens, @nicktrav, and @RaduBerinde)


pkg/cli/start_jemalloc.go line 30 at r1 (raw file):

// // MALLOC_CONF env var. From the jemalloc man page:
// //   The string specified via --with-malloc-conf, the string pointed to by
// //   the global variable malloc_conf, the “name” of the file referenced by

Is this global variable the same as the je_ prefixed one below?

Gemini claimed the following -- wondering whether to believe it:
malloc_conf variable (Deprecated/Rarely Used)

In some (older) versions of jemalloc, there was a global const char * variable called malloc_conf. You might see it referenced in older documentation or code.

This method is generally deprecated and should be avoided in favor of mallctl*

@RaduBerinde
Copy link
Member Author

Yes, it's the same, our build is prepending je_ to all symbols. The latest man page (https://jemalloc.net/jemalloc.3.html, section TUNING) doesn't mention anything about deprecation. Some of the settings are hard or impossible to apply via mallctl after initialization.

@RaduBerinde
Copy link
Member Author

Hm, I see some "Invalid conf pair: muzzy_delay_ms:0" messages in roachtests. I will investigate. Though this is 0 by default in our version anyway so it'd be ok to remove.

@RaduBerinde
Copy link
Member Author

Oh, it's just a typo, it should be decay

This commit changes the jemalloc configuration to reduce Cgo total
spikes. This configuration has shown to reduce spikes without any
noticeable impact to performance.

Epic: none
Release note: None
Copy link
Collaborator

@sumeerbhola sumeerbhola left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @jbowens, @nicktrav, and @RaduBerinde)

@RaduBerinde
Copy link
Member Author

TFTR!

bors r+

@craig craig bot merged commit 49e62ff into cockroachdb:master Jan 25, 2025
22 checks passed
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