fix(memory): enable jemalloc background_thread by default - #1260
Conversation
Adds a `[general]` option (also PGDOG_JEMALLOC_BACKGROUND_THREAD env), default off, that enables jemalloc's background purge threads at startup via tikv-jemalloc-ctl — equivalent to _RJEM_MALLOC_CONF=background_thread:true, so freed memory is returned to the OS after allocation bursts without requiring the env var. No-op on non-jemalloc builds (test/msvc). Refs pgdogdev#1230.
Assert the default (false) and env-var (PGDOG_JEMALLOC_BACKGROUND_THREAD) handling of the new option.
sgrif
left a comment
There was a problem hiding this comment.
This doesn't need to be a config, we should be enabling this by default on all supported platforms. We don't need to be logging errors, this will only return an error on unsupported platforms
|
/cc @levkk Since you asked for this to be a config option. I've never seen a scenario where jemalloc bg threads could be enabled and it wasn't desirable |
|
It was probably pre-second coffee and I wasn't thinking straight. I don't know why bg threads are disabled by default, so I was just thinking maybe we need to find out before unleashing this on everyone, so maybe an env var or something? Not sure. |
|
Yeah, I don't think we need to worry about anything like that. This is pretty universally recommended to be enabled |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Drop the jemalloc_background_thread option, its env var and schema entry; turn background purge threads on at startup instead. Background threads are universally desirable, and mallctl only fails on platforms that don't support them, so the error is ignored rather than logged.
|
@levkk — on "why is this disabled by default": I went through jemalloc's history and sources. The short version is that it's about portability and It was shipped opt-in deliberately. jemalloc 5.0.0 ChangeLog (June 2017): "Implement opt-in per CPU background threads, and use them for asynchronous decay-driven unused dirty page purging." The reasons are visible in the tree:
None of that applies to PgDog: one long-lived process, no Worth noting the default has never been a decision on our side either: jemalloc arrived in #124 as a bare @sgrif — updated as requested. The config option, its env var, the schema entry and the parsing test are gone; background threads are now enabled unconditionally at the top of One implementation note, since the obvious alternative looks simpler than it is: pgdogdev/docs#100 documented the removed option, so I've moved it to draft — there's nothing left for it to document, and I'll close it once this lands. |
|
Ah, this is excellent research, thank you. Exactly what I was looking to understand. In this case, I think we should be able to just merge it as a default on. |
Implements the config-based option suggested in #1230.
Adds a
[general]optionjemalloc_background_thread(defaultfalse, also settable viaPGDOG_JEMALLOC_BACKGROUND_THREAD). When enabled, PgDog turns on jemalloc's background purge threads at startup viatikv-jemalloc-ctl— the programmatic equivalent of_RJEM_MALLOC_CONF=background_thread:true— so freed memory is returned to the OS after bursts of large allocations instead of accumulating as retained dirty pages, without operators having to set the env var.test,msvc)..schema/).Docs: pgdogdev/docs#100.
Refs #1230.