Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Introduce jemalloc-allocator feature flag #6675

Merged
merged 18 commits into from
Feb 9, 2023

Conversation

tonyalaribe
Copy link
Contributor

@tonyalaribe tonyalaribe commented Feb 6, 2023

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:

image

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.

@tonyalaribe tonyalaribe self-assigned this Feb 6, 2023
@tonyalaribe tonyalaribe marked this pull request as ready for review February 6, 2023 14:33
@ordian
Copy link
Member

ordian commented Feb 6, 2023

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

@tonyalaribe
Copy link
Contributor Author

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 mrcnski self-requested a review February 6, 2023 15:59
@mrcnski
Copy link
Contributor

mrcnski commented Feb 6, 2023

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. 😛

@mrcnski mrcnski requested review from ordian and bkchr February 6, 2023 16:11
node/overseer/src/lib.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated
@@ -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 = []
Copy link
Contributor

Choose a reason for hiding this comment

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

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"]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@@ -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)]
Copy link
Contributor

@mrcnski mrcnski Feb 6, 2023

Choose a reason for hiding this comment

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

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;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Remember to remove the commented-out allow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@tonyalaribe 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
Comment on lines 41 to 44
// 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)]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these dead_code allows can be avoided too. 🙂

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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. 🙃

Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Contributor

mrcnski commented Feb 7, 2023

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

@mrcnski
Copy link
Contributor

mrcnski commented Feb 7, 2023

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
Copy link
Contributor

koute commented Feb 7, 2023

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.

Comment on lines 41 to 44
// 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)]
Copy link
Member

Choose a reason for hiding this comment

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

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

Cargo.toml Outdated
@@ -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"]
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@tonyalaribe tonyalaribe changed the title Introduce jemalloc-stats feature flag Introduce jemalloc-allocator feature flag Feb 7, 2023
@ordian 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
@ordian
Copy link
Member

ordian commented Feb 7, 2023

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

@mrcnski
Copy link
Contributor

mrcnski commented Feb 8, 2023

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.

@tonyalaribe
Copy link
Contributor Author

The pipeline passes now. Please review 🙏

@mrcnski
Copy link
Contributor

mrcnski commented Feb 8, 2023

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

Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

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

Comment on lines 638 to 653
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.",
);
()
},
}
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
Copy link
Contributor

@mrcnski mrcnski left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

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
Copy link
Contributor Author

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 06aec1e into master Feb 9, 2023
@paritytech-processbot paritytech-processbot bot deleted the aa/introduce-jemalloc-stats-flag branch February 9, 2023 09:09
mrcnski added a commit that referenced this pull request Feb 9, 2023
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
paritytech-processbot bot pushed a commit that referenced this pull request Feb 14, 2023
* 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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B1-note_worthy Changes should be noted in the release notes 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”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants