feat: auto heap dump by default if `MALLOC_CONF=prof:true` #12186

fuyufjh · 2023-09-08T14:20:07Z

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

As title.

By default, as long as MALLOC_CONF=prof:true is set, auto heap dump will be produced. Otherwise, a line of log will be printed

Cannot dump heap profile because Jemalloc prof is not enabled

This makes it easier to use auto heap dump and also facilitates https://linear.app/risingwave-labs/issue/CLOUD-1791/feature-enable-auto-heap-dump.

The default dump path is inherited from prof.prefix in MALLOC_CONF, but you may override it by the option server.auto_dump_heap_profile.dir. For example:

[server.auto_dump_heap_profile]
enabled = true
dir = "/my/dump/path"
threshold = 0.9

Checklist

I have written necessary rustdoc comments
I have added necessary unit tests and integration tests
I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
My PR contains breaking changes. (If it deprecates some features, please create a tracking issue to remove them in the future).
All checks passed in ./risedev check (or alias, ./risedev c)
My PR changes performance-critical code. (Please run macro/micro-benchmarks and show the results.)

My PR contains critical fixes that are necessary to be merged into the latest release. (Please check out the details)

Documentation

My PR needs documentation updates. (Please use the Release note section below to summarize the impact on users)

Release note

If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.

fuyufjh · 2023-09-08T14:21:25Z

src/compute/src/memory_management/mod.rs

+            convert(total_memory_bytes as _),
+            MIN_COMPUTE_MEMORY_MB
+        );
+    }


Don't know why MIN_COMPUTE_MEMORY_MB was not checked anywhere, so I added it by the way.

KeXiangWang

LGTM!

yuhao-su · 2023-09-08T14:53:47Z

src/common/src/config.rs

@@ -1131,7 +1127,7 @@ pub mod default {

    pub mod auto_dump_heap_profile {
        pub fn dir() -> String {
-            "".to_string()
+            ".".to_string() // current directory


I'll suggest putting it in ./.risingwave/profiling/auto

"." is not perfect but ./.risingwave is only used by risedev (for developers), it seems even worse.

Let me try to make it configurable by env var, so that kube-bench or risedev can set a proper output directory.

It's actually not just used by risedev.

risingwave/src/storage/src/memory.rs

Line 106 in 840114a

create_dir_all("./.risingwave/sled").expect("should create");

But an env for the prefix will be great. It can be useful for putting all kinds of local files.

Also the ci failed because of this change. You need to update the example.toml

It's actually not just used by risedev.

risingwave/src/storage/src/memory.rs

Line 106 in 840114a

create_dir_all("./.risingwave/sled").expect("should create");

But an env for the prefix will be great. It can be useful for putting all kinds of local files.

Hmm, this is not persuasive. memory storage is just for test and playground, and it is never used in production.

At least put it in a directory? Output heap profile in a root directory looks ugly to me. Also we will have a directory for manually dumped in the dashboard pr.

Anyway, I think an env var for the local file prefix would be great. Maybe we can do it in later prs.

I just realized the "env var" actually already exist - MALLOC_CONF

Now it will dump memory profile to prof.prefix if server.auto_dump_heap_profile.dir is absent. Please take a look. 🥰

src/compute/src/memory_management/policy.rs

yuhao-su

LGTM

codecov · 2023-09-09T09:23:47Z

Codecov Report

Merging #12186 (4556122) into main (c924c37) will decrease coverage by 0.02%.
Report is 12 commits behind head on main.
The diff coverage is 18.75%.

@@            Coverage Diff             @@
##             main   #12186      +/-   ##
==========================================
- Coverage   69.76%   69.75%   -0.02%     
==========================================
  Files        1407     1407              
  Lines      235565   235576      +11     
==========================================
- Hits       164351   164317      -34     
- Misses      71214    71259      +45

Flag	Coverage Δ
rust	`69.75% <18.75%> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
...rc/compute/src/memory_management/memory_manager.rs	`0.00% <0.00%> (ø)`
src/compute/src/memory_management/policy.rs	`0.00% <0.00%> (ø)`
src/compute/src/memory_management/mod.rs	`78.61% <37.50%> (+1.40%)`	⬆️
src/common/src/config.rs	`85.63% <100.00%> (+0.49%)`	⬆️

... and 9 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

BugenZhao · 2023-09-11T03:53:03Z

src/common/src/config.rs

@@ -1130,6 +1127,10 @@ pub mod default {
    }

    pub mod auto_dump_heap_profile {
+        pub fn enabled() -> bool {
+            true


Where will we read the env var MALLOC_CONF? 👀

Here: https://github.com/risingwavelabs/risingwave/pull/12186/files#diff-22db760b18e3eab5a13a58cf0ef5249aa2defa83c011f73bf72e41e58bb8363bR131-R132

let prof_prefix_mib = jemalloc_prof::prefix::mib().unwrap(); let prof_prefix = prof_prefix_mib.read().unwrap();

It's actually read from jemalloc lib as its option.

Note that the auto-dump is enabled by default, but if jemalloc's profiling is disabled (which is also the default), nothing happens except the log line mentioned in the PR description.

BugenZhao · 2023-09-11T03:54:28Z

src/cmd/Cargo.toml

@@ -46,7 +46,11 @@ workspace-hack = { path = "../workspace-hack" }
 task_stats_alloc = { path = "../utils/task_stats_alloc" }

 [target.'cfg(unix)'.dependencies]
-tikv-jemallocator = { git = "https://github.com/yuhao-su/jemallocator.git", features = ["profiling", "stats", "unprefixed_malloc_on_supported_platforms"], rev = "a0911601bb7bb263ca55c7ea161ef308fdc623f8" }
+tikv-jemallocator = { git = "https://github.com/risingwavelabs/jemallocator.git", features = [


What about making it a workspace dependency?

risingwave/Cargo.toml

Line 62 in 8841892

[workspace.dependencies]

Let us do it in future PRs. cc. @yuhao-su

…12186)" This reverts commit 17e81b1.

fuyufjh added 2 commits September 8, 2023 18:43

enable auto_dump_heap_profile by default

59e1bb5

check prof is enabled before dump

a4793d7

fuyufjh requested review from yuhao-su and KeXiangWang September 8, 2023 14:20

github-actions bot added the type/feature label Sep 8, 2023

fuyufjh commented Sep 8, 2023

View reviewed changes

KeXiangWang approved these changes Sep 8, 2023

View reviewed changes

yuhao-su reviewed Sep 8, 2023

View reviewed changes

fuyufjh added 2 commits September 9, 2023 08:36

dump to prof.prefix by default

6cf7404

switch jemallocator to risingwavelabs/jemallocator

4556122

fuyufjh requested a review from a team as a code owner September 9, 2023 08:55

yuhao-su approved these changes Sep 9, 2023

View reviewed changes

fuyufjh enabled auto-merge September 9, 2023 09:17

BugenZhao approved these changes Sep 11, 2023

View reviewed changes

fuyufjh added this pull request to the merge queue Sep 11, 2023

Merged via the queue into main with commit 17e81b1 Sep 11, 2023

fuyufjh deleted the eric/auto_dump_if_prof_enabled branch September 11, 2023 04:18

stdrc pushed a commit that referenced this pull request Sep 12, 2023

feat: auto heap dump by default if MALLOC_CONF=prof:true (#12186)

8d1980e

fuyufjh mentioned this pull request Sep 13, 2023

fix: panic when dumping memory profile #12276

Merged

8 tasks

Li0k pushed a commit that referenced this pull request Sep 15, 2023

feat: auto heap dump by default if MALLOC_CONF=prof:true (#12186)

69ea900

fuyufjh added a commit that referenced this pull request Sep 29, 2023

Revert "feat: auto heap dump by default if MALLOC_CONF=prof:true (#…

08822d6

…12186)" This reverts commit 17e81b1.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: auto heap dump by default if `MALLOC_CONF=prof:true` #12186

feat: auto heap dump by default if `MALLOC_CONF=prof:true` #12186

fuyufjh commented Sep 8, 2023 •

edited

Loading

fuyufjh Sep 8, 2023

KeXiangWang left a comment

yuhao-su Sep 8, 2023

fuyufjh Sep 8, 2023

yuhao-su Sep 8, 2023 •

edited

Loading

yuhao-su Sep 8, 2023

fuyufjh Sep 9, 2023 •

edited

Loading

yuhao-su Sep 9, 2023

fuyufjh Sep 9, 2023

yuhao-su left a comment

codecov bot commented Sep 9, 2023 •

edited

Loading

BugenZhao Sep 11, 2023

fuyufjh Sep 11, 2023 •

edited

Loading

BugenZhao Sep 11, 2023

fuyufjh Sep 11, 2023

feat: auto heap dump by default if MALLOC_CONF=prof:true #12186

feat: auto heap dump by default if MALLOC_CONF=prof:true #12186

Conversation

fuyufjh commented Sep 8, 2023 • edited Loading

What's changed and what's your intention?

Checklist

Documentation

Release note

Choose a reason for hiding this comment

KeXiangWang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuhao-su Sep 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fuyufjh Sep 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuhao-su left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 9, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

fuyufjh Sep 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat: auto heap dump by default if `MALLOC_CONF=prof:true` #12186

feat: auto heap dump by default if `MALLOC_CONF=prof:true` #12186

fuyufjh commented Sep 8, 2023 •

edited

Loading

yuhao-su Sep 8, 2023 •

edited

Loading

fuyufjh Sep 9, 2023 •

edited

Loading

codecov bot commented Sep 9, 2023 •

edited

Loading

fuyufjh Sep 11, 2023 •

edited

Loading