feat(build): use tikv-jemalloc only on Linux #7305

ice1000 · 2023-01-11T04:35:14Z

I hereby agree to the terms of the Singularity Data, Inc. Contributor License Agreement.

What's changed and what's your intention?

Exactly as the title said.

Checklist

I have written necessary rustdoc comments
I have added necessary unit tests and integration tests
All checks passed in ./risedev check (or alias, ./risedev c)

Types of user-facing changes

Users can now build this project w/o the tikv-jemalloc compilation failure. /cc https://risingwave-community.slack.com/archives/C02RASFD3MZ/p1672766451385959
However, using RisingWave on non-Linux will still be problematic (see reviews from @fuyufjh)

dep: #7306

fuyufjh · 2023-01-11T05:27:57Z

src/compute/src/memory_management/memory_manager.rs

+    /// Jemalloc is not supported on non-Linux OSs, because tikv-jemalloc is not available.
+    /// See the comments for the macro enable_jemalloc_on_linux!();
+    #[cfg(not(target_os = "linux"))]
+    pub async fn run(self: Arc<Self>, _: Arc<BatchManager>, _: Arc<LocalStreamManager>) {}


This is dangerous. A RisingWave CN instance without memory manager will continue to consume more memory until OOM 🥵

Oh! That's not good. I guess we should warn users about this for non-Linux? I do not expect production-level usages of RisingWave on Windows/macOS, so I think this behavior might be fine (idk, it would be nice if we can implement it with the default allocator)?

It's possible to get rid of jemalloc after the competition of our new memory manager (#7180), because we will only rely on the memory usage collected by ourself (with TaskLocalAlloc)

mistake

codecov · 2023-01-11T06:12:56Z

Codecov Report

Merging #7305 (31906c8) into main (f0e6513) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #7305   +/-   ##
=======================================
  Coverage   73.05%   73.06%           
=======================================
  Files        1068     1069    +1     
  Lines      170985   170982    -3     
=======================================
+ Hits       124913   124920    +7     
+ Misses      46072    46062   -10

Flag	Coverage Δ
rust	`73.06% <100.00%> (+<0.01%)`	⬆️

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

Impacted Files	Coverage Δ
src/cmd/src/bin/compactor.rs	`100.00% <ø> (ø)`
src/cmd/src/bin/ctl.rs	`100.00% <ø> (ø)`
src/cmd/src/bin/frontend_node.rs	`100.00% <ø> (ø)`
src/cmd_all/src/bin/risingwave.rs	`1.38% <ø> (-1.36%)`	⬇️
src/common/src/lib.rs	`100.00% <ø> (ø)`
...rc/compute/src/memory_management/memory_manager.rs	`0.00% <ø> (ø)`
src/common/src/jemalloc.rs	`100.00% <100.00%> (ø)`
src/common/src/cache.rs	`97.31% <0.00%> (-0.23%)`	⬇️
src/storage/src/hummock/compactor/mod.rs	`83.33% <0.00%> (-0.16%)`	⬇️
src/stream/src/executor/aggregation/minput.rs	`96.49% <0.00%> (+0.10%)`	⬆️
... and 2 more

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

TennyZhuang · 2023-01-11T06:59:36Z

Generally LGTM, and remove GlobalMemoryManager on non-production platform seems acceptable to me.

BTW, there are some better solutions:

Find an alternative solution to achieve the global memory usage without the dependency to jemalloc. (hard)
Add a feature jemalloc, and let windows user disable it manually.

Anyway, the PR looks good enough to me, I don’t expect users to run heavy workloads on macOS.

ice1000 · 2023-01-11T07:25:10Z

It's up to the moderators to decide if this is good to go 😉

fuyufjh

Acceptable to me because eventually #7180 will fix it ☺️

fuyufjh · 2023-01-11T10:33:23Z

src/compute/src/memory_management/memory_manager.rs

@@ -71,6 +71,7 @@ impl GlobalMemoryManager {

    /// Jemalloc is not supported on non-Linux OSs, because tikv-jemalloc is not available.
    /// See the comments for the macro enable_jemalloc_on_linux!();
+    // FIXME: remove such limitation after #7180


cc. @BowenXiao1999

yuhao-su · 2023-01-12T08:11:09Z

The jemalloc issue only mentions the compilation problem on Windows. Should we also allow jemalloc on macOS?

ice1000 · 2023-01-12T08:12:05Z

The jemalloc issue only mentions the compilation problem on Windows. Should we also allow jemalloc on macOS?

Does tikv jemalloc support macOS? Because I'm not sure about that.

yuhao-su · 2023-01-12T08:16:53Z

The jemalloc issue only mentions the compilation problem on Windows. Should we also allow jemalloc on macOS?

Does tikv jemalloc support macOS? Because I'm not sure about that.

Yes. It works fine on macOS since the first day we add jemalloc.

ice1000 · 2023-01-12T08:39:37Z

The jemalloc issue only mentions the compilation problem on Windows. Should we also allow jemalloc on macOS?

Does tikv jemalloc support macOS? Because I'm not sure about that.

Yes. It works fine on macOS since the first day we add jemalloc.

Okay. I'll make another pr.

BugenZhao · 2023-01-12T08:42:39Z

Previously I thought there were some new problems on macOS. 😄 RisingWave won't guarantee that it works on a non-Unix system, and risedev requires the bash as well. Please use the cfg(unix) if you think this really matters.

ice1000 · 2023-01-12T14:36:50Z

I think I saw a user having macOS tikv-jemalloc issues in a community channel...

yuhao-su · 2023-01-12T14:43:04Z

I think I saw a user having macOS tikv-jemalloc issues in a community channel...

Latest try was to spin up a docker container on ubuntu.20.04 and exec into that. There I could set up the dev env needed and build everything BUT tikv-jemalloc-sys wher I failed on a build script.
error: failed to run custom build command for tikv-jemalloc-sys v0.5.2+5.3.0-patched
Caused by:
process didn't exit successfully: /risingwave/target/debug/build/tikv-jemalloc-sys-c28ff9413adad8ce/build-script-build (exit status: 101)

You mean this? I thought he is having problem compiling on ubuntu..

ice1000 · 2023-01-12T15:03:23Z

Yes, I meant this. They mentioned macOS in another message

BowenXiao1999 · 2023-01-13T02:39:48Z

Add a data point:

According to the experiment with Mac M1, the number observed in Mac M1 seems have some problem: jemalloc much higher than node memory. Switch to ubuntu fix this.

But I'm not sure Mac Intel.

#7180 (comment)

Fix the warnings introduced by #7305 Approved-By: st1page Approved-By: yuhao-su Approved-By: BugenZhao Approved-By: BowenXiao1999 Approved-By: ice1000

ice1000 added 3 commits January 11, 2023 04:29

Define enable_jemalloc_on_linux

65825f2

Linux_only

90564a2

Fmt

9be7499

github-actions bot added the type/feature label Jan 11, 2023

ice1000 added 5 commits January 11, 2023 04:47

lol

319203c

Removal

92a28c9

Merge: branch 'tiz/remove' into tiz/jemalloc

587868f

Linux-only

ee357e6

Rebase: remote-tracking branch 'origin/main' into tiz/jemalloc

bf47c47

fuyufjh previously approved these changes Jan 11, 2023

View reviewed changes

ice1000 added 4 commits January 11, 2023 05:29

Really Linux-only

6cd34b0

Rebase remote-tracking branch 'origin/main' into tiz/jemalloc

118f89a

Hakari

7d7e82d

Wow

c3f0e58

ice1000 requested review from TennyZhuang and yingjunwu January 11, 2023 05:52

st1page requested a review from BugenZhao January 11, 2023 06:22

Rebase remote-tracking branch 'origin/main' into tiz/jemalloc

61bc8fd

ice1000 added the mergify/can-merge label Jan 11, 2023

fuyufjh approved these changes Jan 11, 2023

View reviewed changes

mergify bot and others added 2 commits January 11, 2023 10:31

Merge branch 'main' into tiz/jemalloc

f9c55ef

add a FIXME there

31906c8

fuyufjh reviewed Jan 11, 2023

View reviewed changes

mergify bot merged commit 68b0111 into main Jan 11, 2023

mergify bot deleted the tiz/jemalloc branch January 11, 2023 10:55

BowenXiao1999 mentioned this pull request Jan 12, 2023

feat: collect stream mem usage #7180

Merged

3 tasks

ice1000 mentioned this pull request Jan 12, 2023

Allow jemalloc on macOS? #7358

Closed

ice1000 mentioned this pull request Jan 13, 2023

chore(*): fix clippy warning #7383

Merged

4 tasks

TennyZhuang mentioned this pull request Jan 16, 2023

chore(*): fix warnings on non-linux platforms #7397

Merged

4 tasks

mergify bot pushed a commit that referenced this pull request Jan 16, 2023

chore(*): fix warnings on non-linux platforms (#7397)

b63df21

Fix the warnings introduced by #7305 Approved-By: st1page Approved-By: yuhao-su Approved-By: BugenZhao Approved-By: BowenXiao1999 Approved-By: ice1000

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(build): use tikv-jemalloc only on Linux #7305

feat(build): use tikv-jemalloc only on Linux #7305

ice1000 commented Jan 11, 2023 •

edited

Loading

fuyufjh Jan 11, 2023 •

edited

Loading

ice1000 Jan 11, 2023

fuyufjh Jan 11, 2023

codecov bot commented Jan 11, 2023 •

edited

Loading

TennyZhuang commented Jan 11, 2023

ice1000 commented Jan 11, 2023

fuyufjh left a comment

fuyufjh Jan 11, 2023

yuhao-su commented Jan 12, 2023

ice1000 commented Jan 12, 2023

yuhao-su commented Jan 12, 2023

ice1000 commented Jan 12, 2023

BugenZhao commented Jan 12, 2023

ice1000 commented Jan 12, 2023

yuhao-su commented Jan 12, 2023 •

edited

Loading

ice1000 commented Jan 12, 2023

BowenXiao1999 commented Jan 13, 2023

feat(build): use tikv-jemalloc only on Linux #7305

feat(build): use tikv-jemalloc only on Linux #7305

Conversation

ice1000 commented Jan 11, 2023 • edited Loading

What's changed and what's your intention?

Checklist

Types of user-facing changes

fuyufjh Jan 11, 2023 • edited Loading

Choose a reason for hiding this comment

ice1000 Jan 11, 2023

Choose a reason for hiding this comment

fuyufjh Jan 11, 2023

Choose a reason for hiding this comment

codecov bot commented Jan 11, 2023 • edited Loading

Codecov Report

TennyZhuang commented Jan 11, 2023

ice1000 commented Jan 11, 2023

fuyufjh left a comment

Choose a reason for hiding this comment

fuyufjh Jan 11, 2023

Choose a reason for hiding this comment

yuhao-su commented Jan 12, 2023

ice1000 commented Jan 12, 2023

yuhao-su commented Jan 12, 2023

ice1000 commented Jan 12, 2023

BugenZhao commented Jan 12, 2023

ice1000 commented Jan 12, 2023

yuhao-su commented Jan 12, 2023 • edited Loading

ice1000 commented Jan 12, 2023

BowenXiao1999 commented Jan 13, 2023

ice1000 commented Jan 11, 2023 •

edited

Loading

fuyufjh Jan 11, 2023 •

edited

Loading

codecov bot commented Jan 11, 2023 •

edited

Loading

yuhao-su commented Jan 12, 2023 •

edited

Loading