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

Add custom Ying memory profiler and prevent giant allocations #5649

Merged
merged 3 commits into from
Oct 30, 2022

Conversation

velvia
Copy link
Contributor

@velvia velvia commented Oct 28, 2022

This change swaps out the explicit Jemalloc-based memory profiling code with my custom Ying profiler (https://github.com/velvia/ying-profiler), using the GlobalAlloc trait. Why?

  1. This lets us get much richer and better Rust stack traces for the largest sources of retained memory from Rust code, esp async code, so we can pinpoint where the largest sources of allocation are happening, in a way that jeprof is not able to obtain;
  2. This will allow us to prevent large memory allocations that is messing up devnet
  3. Memory profiling is still using a sampling strategy, so it should remain light in CPU usage.

I have run local stress benchmark with the profiler turned on, and the QPS remained about the same as before (roughly ~360 when target = 400).

NOTE: the large memory allocation prevention will be updated in this PR shortly by uopdating the git pointer

This does not prevent one from using jemalloc, which can still be used by adding LD_PRELOAD and setting MALLOC_CONF etc. It does switch the default to the default Rust memory allocator underneath though.

@longbowlu
Copy link
Collaborator

It does switch the default to the default Rust memory allocator underneath though.

@mwtian it seems to collide with #5614 ?

crates/sui-node/src/main.rs Show resolved Hide resolved
Copy link
Contributor

@mwtian mwtian left a comment

Choose a reason for hiding this comment

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

Looks good. This will disable process level jemalloc profiling in devnet. But depending on the Ying allocator is implemented, jemalloc profiling is probably still enabled in private testnet which would be fine.

@@ -51,9 +51,5 @@ COPY --from=builder /sui/target/release/sui-node /usr/local/bin

ARG BUILD_DATE
ARG GIT_REVISION
# Use jemalloc as memory allocator
ENV LD_PRELOAD /usr/lib/x86_64-linux-gnu/libjemalloc.so
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this removed to get clearer signal, or you have found incompatibilities? In the longer term we probably want to use jemalloc regardless of its profiling 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.

Yeah this is just so that we can do it environment by environment, to be safer

@velvia velvia enabled auto-merge (squash) October 28, 2022 22:18
@velvia velvia force-pushed the ec/wip-ying-memory-profiler branch from 0275e24 to 594b5f4 Compare October 28, 2022 22:23
@velvia velvia merged commit ef090e1 into main Oct 30, 2022
@velvia velvia deleted the ec/wip-ying-memory-profiler branch October 30, 2022 04:28
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.

4 participants