Introduce jemalloc-allocator feature flag #6675

tonyalaribe · 2023-02-06T14:08:42Z

Background

polkadot currently relies on the jemalloc allocator via the tikv-jemallocator library. Unfortunately jemalloc's support is not equally great accross all platforms. Even the jemallocator page shows that outside of x84_64 linux architector, a lot of their tests fail on the other platforms:

The direct effect of this is that running polkadot on some platforms result in runtime errors due to jemalloc:
<jemalloc>: Error Allocating TBD

Context

For context on the issue, look at the existing github issue: #6591

Implementation

Since polkadot primarily supports linux, this PR introduces the jemalloc-allocator feature flag which can be used to explicitly enable the jemalloc allocator and it's stats collection functionality, but otherwise this allocator and functionality is disabled except on linux.

ordian · 2023-02-06T15:31:11Z

IMHO we should just enable both jemalloc and jemalloc stats on linux and disable it everywhere else. No additional feature.

tonyalaribe · 2023-02-06T15:35:18Z

IMHO we should just enable both jemalloc and jemalloc stats on linux and disable it everywhere else. No additional feature.

Interesting. I'm not sure what the others think, but I'll effect this and see.

mrcnski · 2023-02-06T16:09:02Z

IMHO we should just enable both jemalloc and jemalloc stats on linux and disable it everywhere else. No additional feature.

@ordian Hmm, but it seems to work for most Mac users (and it works for me on the Macbooks I've tried). jemalloc is supposed to have more predictable performance, so I think it's worth enabling it where we can. Having a feature which can be toggled off for the default allocator makes sense to me.

That said, I'm not sure how much the failing tests on Mac matter. Might be worth more research into which tests fail and whether it is likely to affect our users on Mac.

Small aside, we currently collect memory stats using jemalloc, including the preparation memory metrics. If we disable jemalloc, with no option to enable on Mac, then I wouldn't be able to collect memory metrics locally anymore. 😛

node/overseer/src/lib.rs

mrcnski · 2023-02-06T16:18:03Z

Cargo.toml

@@ -202,6 +202,7 @@ try-runtime = [ "polkadot-cli/try-runtime" ]
 fast-runtime = [ "polkadot-cli/fast-runtime" ]
 runtime-metrics = [ "polkadot-cli/runtime-metrics" ]
 pyroscope = ["polkadot-cli/pyroscope"]
+jemalloc-stats = []


You can make the tikv-jemallocator dependency optional and then enable it here (untested, but I think it works):

Suggested change

jemalloc-stats = []

jemalloc-stats = ["tikv-jemallocator"]

Actually i just realized that making the dependency optional and enabled only by the flag would make it break on linux, where we want it available whether the flag is present or not.

What do you think? I'm undoing the change i made for now.

Actually i just realized that making the dependency optional and enabled only by the flag would make it break on linux, where we want it available whether the flag is present or not.

You can add a Linux-only section in Cargo.toml to enable it unconditionally on Linux, e.g. I'd suggest this:

[dependencies] tikv-jemallocator = { version = "0.5.0", optional = true } [target.'cfg(target_os = "linux")'.dependencies] tikv-jemallocator = "0.5.0" [features] force-use-jemalloc = ["dep:tikv-jemallocator"]

Then jemalloc will be always enabled on Linux, and on other platforms you can enable it though force-use-jemalloc feature.

Thanks @koute, I tried that and it seems to work nicely. Thanks!
Btw, could you look at the PR again?

mrcnski · 2023-02-06T16:19:19Z

node/overseer/src/memory_stats.rs

@@ -14,7 +14,10 @@
 // You should have received a copy of the GNU General Public License
 // along with Polkadot.  If not, see <http://www.gnu.org/licenses/>.

-use tikv_jemalloc_ctl::{epoch, stats, Error};
+// Allow unused code as they might be enabled only for the jemalloc-stats feature flag.
+#![allow(dead_code)]


Instead of allowing dead code, you can gate the module on conditional compilation as you do elsewhere, i.e. with something like this in lib.rs:

#[cfg(any(target_os = "linux", feature = "jemalloc-stats"))] mod memory_stats;

I didn't expect this to work because i tried something similar before. But it worked nicely. So i've pushed the update.

Remember to remove the commented-out allow.

Yeah, i've removed it. Thanks for pointing it out

Co-authored-by: Marcin S. <marcin@bytedude.com>

mrcnski · 2023-02-07T10:12:11Z

node/overseer/src/metrics.rs

+	// Allow unused code as they might be enabled only for the jemalloc-stats feature flag.
+	#[allow(dead_code)]
 	memory_stats_resident: prometheus::Gauge<prometheus::U64>,
+	#[allow(dead_code)]


I think these dead_code allows can be avoided too. 🙂

When they are not available, we they should not be registered in prometheus. Aka also put a cfg above them

Yeah, that makes sense. I've made the change @bkchr. Could you take a look again?

When they are not available, we they should not be registered in prometheus. Aka also put a cfg above them

What is the harm in registering unavailable metrics, if we never call them? (I believe you, I actually don't know the answer.)

PVF has some metrics that would need this treatment: preparation_max_rss, preparation_max_allocated, preparation_max_resident. I can take care of that, but honestly was hoping to get away without ugly #[cfg] directives everywhere. 🙃

What is the harm in registering unavailable metrics, if we never call them? (I believe you, I actually don't know the answer.)

There is no real harm, just people that will ask why the metric is zero :P Metric being not there is IMO better than just not working.

mrcnski · 2023-02-07T10:14:10Z

I pushed a change applying the conditional compilation to the preparation memory tracker.

mrcnski · 2023-02-07T12:43:17Z

Oh yeah, another thing to note:

Right now we are using jemalloc during PVF preparation only for collecting memory metrics. As such, being able to produce the metrics is optional right now.

However, in the future we may need these stats during pre-checking, to vote against PVFs that use too much memory during preparation. In that case, having jemalloc enabled would be a soft requirement for validators, in order to participate in the memory check. I say "soft" because I believe there is no penalty for being on the wrong side of a pre-checking vote. Still, if we made this change in the future, we would want some check in the code that validators are either Linux or running with this feature.

koute · 2023-02-07T12:57:42Z

However, in the future we may need these stats during pre-checking, to vote against PVFs that use too much memory during preparation. In that case, having jemalloc enabled would be a soft requirement for validators, in order to participate in the memory check. I say "soft" because I believe there is no penalty for being on the wrong side of a pre-checking vote. Still, if we made this change in the future, we would want some check in the code that validators are either Linux or running with this feature.

We could perhaps emulate this by having a global allocator wrapper and just tracking the sizes ourselves. Won't be as accurate since it wouldn't include the allocator's overhead/metadata, but on other hand that'd make it more consistent (the figures reported would be the same on different allocators) so maybe it's not a bad thing.

bkchr · 2023-02-07T13:11:44Z

node/overseer/src/metrics.rs

+	// Allow unused code as they might be enabled only for the jemalloc-stats feature flag.
+	#[allow(dead_code)]
 	memory_stats_resident: prometheus::Gauge<prometheus::U64>,
+	#[allow(dead_code)]


When they are not available, we they should not be registered in prometheus. Aka also put a cfg above them

bkchr · 2023-02-07T13:12:45Z

Cargo.toml

@@ -202,6 +206,7 @@ try-runtime = [ "polkadot-cli/try-runtime" ]
 fast-runtime = [ "polkadot-cli/fast-runtime" ]
 runtime-metrics = [ "polkadot-cli/runtime-metrics" ]
 pyroscope = ["polkadot-cli/pyroscope"]
+jemalloc-stats = ["polkadot-node-core-pvf/jemalloc-stats", "polkadot-overseer/jemalloc-stats"]


Please rename this to jemalloc or jemalloc-allocator and then do the trick that @koute mentioned with enabling it automatically for Linux.

Yeah, i've done this. Thanks!! Please review again

ordian · 2023-02-07T16:20:20Z

Doesn't compile yet, but looks good to me.

mrcnski · 2023-02-08T09:52:14Z

We could perhaps emulate this by having a global allocator wrapper and just tracking the sizes ourselves. Won't be as accurate since it wouldn't include the allocator's overhead/metadata, but on other hand that'd make it more consistent (the figures reported would be the same on different allocators) so maybe it's not a bad thing.

Interesting idea, will extract into a follow-up issue.

node/overseer/src/metrics.rs

Co-authored-by: Marcin S. <marcin@bytedude.com>

…ech/polkadot into aa/introduce-jemalloc-stats-flag

tonyalaribe · 2023-02-08T14:34:21Z

The pipeline passes now. Please review 🙏

mrcnski · 2023-02-08T14:51:37Z

Looks 99% good to me @tonyalaribe. I might need to make another update based on #6675 (comment) though.

bkchr

Most of the stuff looks good, but you added some bug. That needs to be fixed.

bkchr · 2023-02-08T15:09:58Z

node/overseer/src/lib.rs

+	match MemoryAllocationTracker::new() {
+		Ok(memory_stats) => match memory_stats.snapshot() {
+			Ok(memory_stats_snapshot) => {
+				gum::trace!(target: LOG_TARGET, "memory_stats: {:?}", &memory_stats_snapshot);
+				metrics.memory_stats_snapshot(memory_stats_snapshot);
+			},
+			Err(e) => gum::debug!(target: LOG_TARGET, "Failed to obtain memory stats: {:?}", e),
+		},
+		Err(_) => {
+			gum::debug!(
+				target: LOG_TARGET,
+				"Memory allocation tracking is not supported by the allocator.",
+			);
+			()
+		},
+	}


This is wrong as you now always recreate MemoryAllocationTracker. Before it was created once and then "attached" to the closure.

Thanks for pointing it out. I reverted to defining the collect_memory_stats closure inline like it was before. Could you take a look again?

node/overseer/src/lib.rs

Co-authored-by: Bastian Köcher <git@kchr.de>

…ech/polkadot into aa/introduce-jemalloc-stats-flag

mrcnski

Excellent work!

I was going to fix #6675 (comment) in this PR, but it introduced too many unrelated changes. 😅 So I'll raise a separate PR -- this one is already good as-is.

tonyalaribe · 2023-02-09T09:08:51Z

Excellent work!

I was going to fix #6675 (comment) in this PR, but it introduced too many unrelated changes. 😅 So I'll raise a separate PR -- this one is already good as-is.

Thanks @mrcnski! Especially for helping shape this issue and PR.
I’ll go ahead and merge this now.

tonyalaribe · 2023-02-09T09:09:02Z

bot merge

The original purpose of this change was to gate metrics that are unsupported by some systems behind conditional compilation directives (#[cfg]); see #6675 (comment). Then I started doing some random cleanups and simplifications and got a bit carried away. 🙈 The code should be overall tidier than before. Changes: - Don't register unsupported metrics (e.g. `max_rss` on non-Linux systems) - Introduce `PrepareStats` struct as an abstraction over the `Ok` values of `PrepareResult`. It is cleaner, and can be easily modified in the future. - Other small changes

* Refactor PVF preparation memory stats The original purpose of this change was to gate metrics that are unsupported by some systems behind conditional compilation directives (#[cfg]); see #6675 (comment). Then I started doing some random cleanups and simplifications and got a bit carried away. 🙈 The code should be overall tidier than before. Changes: - Don't register unsupported metrics (e.g. `max_rss` on non-Linux systems) - Introduce `PrepareStats` struct as an abstraction over the `Ok` values of `PrepareResult`. It is cleaner, and can be easily modified in the future. - Other small changes * Minor fixes to comments * Fix compile errors * Try to fix some Linux errors * Mep * Fix candidate-validation tests * Update docstring

tonyalaribe added 2 commits February 6, 2023 14:58

Introduce jemalloc-stats feature flag

748085d

remove unneeded space

414f5fe

tonyalaribe self-assigned this Feb 6, 2023

tonyalaribe marked this pull request as ready for review February 6, 2023 14:33

mrcnski self-requested a review February 6, 2023 15:59

mrcnski requested review from ordian and bkchr February 6, 2023 16:11

mrcnski reviewed Feb 6, 2023

View reviewed changes

node/overseer/src/lib.rs Outdated Show resolved Hide resolved

mrcnski reviewed Feb 6, 2023

View reviewed changes

tonyalaribe and others added 4 commits February 6, 2023 17:38

Update node/overseer/src/lib.rs

f18011b

Co-authored-by: Marcin S. <marcin@bytedude.com>

Update Cargo.toml

ccec741

Co-authored-by: Marcin S. <marcin@bytedude.com>

revert making tikv-jemallocator depend on jemalloc-stats

af996ba

conditionally import memory_stats instead of using dead_code

3e611a3

tonyalaribe added A0-please_review Pull request needs code review. B1-note_worthy Changes should be noted in the release notes labels Feb 6, 2023

tonyalaribe and others added 4 commits February 6, 2023 18:43

fix test via expllicit import

a0ece33

Merge branch 'master' into aa/introduce-jemalloc-stats-flag

1370166

Add jemalloc-stats feature to crates, propagate it from root

e2b890a

Apply jemalloc-stats feature to prepare mem stats; small refactor

326b44a

mrcnski reviewed Feb 7, 2023

View reviewed changes

bkchr reviewed Feb 7, 2023

View reviewed changes

tonyalaribe added 2 commits February 7, 2023 15:15

effect changes recommended on PR

5fd13c7

Merge branch 'master' into aa/introduce-jemalloc-stats-flag

3cf7225

tonyalaribe changed the title ~~Introduce jemalloc-stats feature flag~~ Introduce jemalloc-allocator feature flag Feb 7, 2023

ordian approved these changes Feb 7, 2023

View reviewed changes

ordian added C1-low PR touches the given topic and has a low impact on builders. T0-node This PR/Issue is related to the topic “node”. labels Feb 7, 2023

mrcnski mentioned this pull request Feb 8, 2023

Track PVF preparation memory with a global allocator wrapper paritytech/polkadot-sdk#718

Closed

mrcnski reviewed Feb 8, 2023

View reviewed changes

node/overseer/src/metrics.rs Outdated Show resolved Hide resolved

tonyalaribe and others added 3 commits February 8, 2023 12:58

Update node/overseer/src/metrics.rs

c65001b

Co-authored-by: Marcin S. <marcin@bytedude.com>

fix compile error on in pipeline for linux. missing import

496173b

Merge branch 'aa/introduce-jemalloc-stats-flag' of github.com:parityt…

91947fb

…ech/polkadot into aa/introduce-jemalloc-stats-flag

bkchr suggested changes Feb 8, 2023

View reviewed changes

tonyalaribe and others added 3 commits February 8, 2023 16:38

Update node/overseer/src/lib.rs

5127ea7

Co-authored-by: Bastian Köcher <git@kchr.de>

revert to defining collect_memory_stats inline

be7c950

Merge branch 'aa/introduce-jemalloc-stats-flag' of github.com:parityt…

a745219

…ech/polkadot into aa/introduce-jemalloc-stats-flag

bkchr approved these changes Feb 8, 2023

View reviewed changes

mrcnski approved these changes Feb 9, 2023

View reviewed changes

paritytech-processbot bot merged commit 06aec1e into master Feb 9, 2023

paritytech-processbot bot deleted the aa/introduce-jemalloc-stats-flag branch February 9, 2023 09:09

mrcnski mentioned this pull request Feb 9, 2023

Refactor PVF preparation memory stats #6693

Merged

kiltbot mentioned this pull request Apr 3, 2023

[AUTOMATIC] Update Polkadot dependencies from 0.9.38 to 0.9.39 KILTprotocol/kilt-node#495

Closed

librelois mentioned this pull request Apr 6, 2023

Update substrate/polkadot/cumulus from v0.9.38 to v0.9.40 moonbeam-foundation/moonbeam#2200

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce jemalloc-allocator feature flag #6675

Introduce jemalloc-allocator feature flag #6675

tonyalaribe commented Feb 6, 2023 •

edited

Loading

ordian commented Feb 6, 2023

tonyalaribe commented Feb 6, 2023

mrcnski commented Feb 6, 2023 •

edited

Loading

mrcnski Feb 6, 2023

tonyalaribe Feb 6, 2023

tonyalaribe Feb 6, 2023

koute Feb 7, 2023

tonyalaribe Feb 7, 2023

mrcnski Feb 6, 2023 •

edited

Loading

tonyalaribe Feb 6, 2023

mrcnski Feb 7, 2023

tonyalaribe Feb 7, 2023

mrcnski Feb 7, 2023

bkchr Feb 7, 2023

tonyalaribe Feb 7, 2023

mrcnski Feb 8, 2023

bkchr Feb 8, 2023

mrcnski commented Feb 7, 2023

mrcnski commented Feb 7, 2023

koute commented Feb 7, 2023

bkchr Feb 7, 2023

bkchr Feb 7, 2023

tonyalaribe Feb 7, 2023

ordian commented Feb 7, 2023

mrcnski commented Feb 8, 2023

tonyalaribe commented Feb 8, 2023

mrcnski commented Feb 8, 2023

bkchr left a comment

bkchr Feb 8, 2023

tonyalaribe Feb 8, 2023

mrcnski left a comment

tonyalaribe commented Feb 9, 2023

tonyalaribe commented Feb 9, 2023

Introduce jemalloc-allocator feature flag #6675

Introduce jemalloc-allocator feature flag #6675

Conversation

tonyalaribe commented Feb 6, 2023 • edited Loading

Background

Context

Implementation

ordian commented Feb 6, 2023

tonyalaribe commented Feb 6, 2023

mrcnski commented Feb 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrcnski Feb 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrcnski commented Feb 7, 2023

mrcnski commented Feb 7, 2023

koute commented Feb 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ordian commented Feb 7, 2023

mrcnski commented Feb 8, 2023

tonyalaribe commented Feb 8, 2023

mrcnski commented Feb 8, 2023

bkchr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrcnski left a comment

Choose a reason for hiding this comment

tonyalaribe commented Feb 9, 2023

tonyalaribe commented Feb 9, 2023

tonyalaribe commented Feb 6, 2023 •

edited

Loading

mrcnski commented Feb 6, 2023 •

edited

Loading

mrcnski Feb 6, 2023 •

edited

Loading