fix(java_binding): fix java binding TLS allocation failure #12862

chenzl25 · 2023-10-16T06:37:47Z

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

What's changed and what's your intention?

We have encountered a TLS allocation failure in java_binding test and connector node integration test https://buildkite.com/risingwavelabs/main-cron/builds/913#018b1611-bb03-459f-9948-4b8932babd9c
After a basic investigation, we found refactor(heap_profiling): use background heap profiling and enable auto dump for compute and compactor #12694 introduced a jemalloc_ctl in the common crate and our Java dynamic library relies on it.
We found that jemalloc_ctl will register some global symbols in the Java dynamic library which are unexpected.

nm -gUj librisingwave_java_binding.dylib

_JNI_OnLoad
_Java_com_risingwave_java_binding_Binding_iteratorClose
_Java_com_risingwave_java_binding_Binding_iteratorGetArrayValue
_Java_com_risingwave_java_binding_Binding_iteratorGetBooleanValue
...
__rjem_je_arena_bin_offsets
__rjem_je_arena_binind_div_info
__rjem_je_arena_emap_global
__rjem_je_arena_pa_central_global
__rjem_je_arenas
__rjem_je_arenas_lock

...

Based on this issue in jemalloc jemalloc 5.0.1 TLS error: cannot allocate memory in static TLS block jemalloc/jemalloc#1237 this PR tries to enable disable_initial_exec_tls this feature when we build the Java dynamic library to solve the problem.
Finally, we decided to separate the jemalloc_ctl from our common crate.

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.

codecov · 2023-10-16T06:59:14Z

Codecov Report

Merging #12862 (3c037ff) into main (5d97d60) will increase coverage by 0.05%.
Report is 34 commits behind head on main.
The diff coverage is 93.73%.

@@            Coverage Diff             @@
##             main   #12862      +/-   ##
==========================================
+ Coverage   69.14%   69.19%   +0.05%     
==========================================
  Files        1487     1489       +2     
  Lines      244648   245776    +1128     
==========================================
+ Hits       169150   170055     +905     
- Misses      75498    75721     +223

Flag	Coverage Δ
rust	`69.19% <93.73%> (+0.05%)`	⬆️

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

Files	Coverage Δ
src/common/heap_profiling/src/jeprof.rs	`0.00% <ø> (ø)`
src/common/heap_profiling/src/profiler.rs	`0.00% <ø> (ø)`
src/compute/src/server.rs	`0.00% <ø> (ø)`
src/connector/src/lib.rs	`52.08% <ø> (ø)`
src/connector/src/sink/encoder/json.rs	`84.63% <100.00%> (ø)`
src/connector/src/sink/mod.rs	`62.04% <ø> (+0.72%)`	⬆️
src/expr/core/src/aggregate/def.rs	`69.66% <ø> (ø)`
.../frontend/src/optimizer/plan_node/generic/union.rs	`100.00% <100.00%> (ø)`
.../frontend/src/optimizer/plan_node/logical_union.rs	`97.09% <100.00%> (+0.17%)`	⬆️
src/meta/src/dashboard/mod.rs	`0.00% <ø> (ø)`
... and 9 more

... and 29 files with indirect coverage changes

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

StrikeW · 2023-10-17T02:55:55Z

Hi @chenzl25, is this a critical issue that will block the release of v1.3? After v1.3, I think we will switch to the embedded jvm totally.

chenzl25 · 2023-10-17T03:51:33Z

Hi @chenzl25, is this a critical issue that will block the release of v1.3? After v1.3, I think we will switch to the embedded jvm totally.

This PR doesn't fix the issue, we need further investigations. I think it is not a critical blocker. Yes, we have switched to the embedded jvm totally.

…g_TLS_allocation_failure

This reverts commit fe6820b.

fuyufjh · 2023-10-17T07:10:21Z

Can you please explain some context in PR description?

BugenZhao · 2023-10-17T07:12:07Z

src/common/Cargo.toml

+[features]
+default = []
+disable_initial_exec_tls = ["tikv-jemalloc-ctl/disable_initial_exec_tls"]


Note that this will be enabled in the binary level, so all crates will be affected. Maybe you can directly add it here if it's expected.

risingwave/Cargo.toml

Line 119 in d2ec83c

tikv-jemalloc-ctl = { git = "https://github.com/risingwavelabs/jemallocator.git", rev = "64a2d9" }

disable_initial_exec_tls will only be enabled when build java binding, will it still affect rw binary?

will only be enabled when build java binding

This isn't true, as it's unified by hakari. You need to exclude tikv-jemalloc-ctl or risingwave_java_binding completely from hakari if you want to do so.

I want to enable the disable_initial_exec_tls feature only when we build the Java dynamic library. In other cases, the default feature is ok.

A little explanation: hakari tries to help you use the same feature sets regardless of what pkg you use in cargo build -p <pkg>, in order to avoid re-compilation. But you want exactly the reverse here: turn on different features for different pkgs.

I do think so, let me try. Not quite familiar with hakari before.

By the way, I find java_binding does not utilize jemalloc at all but suffers from its issue. It might not be be a good idea for common to depend on jemalloc.

Note that in this case, there will be two malloc implementations operating in the same process, which will almost certainly result in confusing runtime crashes if pointers leak from one implementation to the other.

It might not be be a good idea for common to depend on jemalloc.

Storage depends on jemalloc profiling, which has to depend on jemalloc. We may need to remove the profiling from compactor to address this.

It might not be be a good idea for common to depend on jemalloc.

Storage depends on jemalloc profiling, which has to depend on jemalloc. We may need to remove the profiling from compactor to address this.

+1 to remove it from common (to risingwave_common_heap_profiling). #11665

chenzl25 · 2023-10-17T09:09:23Z

@BugenZhao @xxchan PTAL again~

fuyufjh

LGTM as it only affects java-binding

xxchan · 2023-10-17T09:37:37Z

It might not be be a good idea for common to depend on jemalloc.

Storage depends on jemalloc profiling, which has to depend on jemalloc. We may need to remove the profiling from compactor to address this.

+1 to remove it from common (to risingwave_common_heap_profiling). #11665

I just pushed this alternative solution, which I believe is the better way to solve this issue.

yuhao-su · 2023-10-17T09:44:12Z

I just pushed this alternative solution, which I believe is the better way to solve this issue.

this LGTM too

StrikeW · 2023-10-17T10:12:21Z

Shall we pick this PR to v1.3?

chenzl25 · 2023-10-17T10:15:40Z

Shall we pick this PR to v1.3?

It just affects our testing. While considering we might run a test on v1.3 after some cherry-picks, we can cherry-pick this PR to v1.3.

StrikeW · 2023-10-17T10:28:51Z

Shall we pick this PR to v1.3?

It just affects our testing. While considering we might run a test on v1.3 after some cherry-picks, we can cherry-pick this PR to v1.3.

I checked that the java binding .so only loaded in non embedded jvm mode, so it won't affect the functionality of connector.

fix

fe6820b

chenzl25 added ci/run-java-binding-tests ci/run-java-connector-node-integration-tests labels Oct 16, 2023

github-actions bot added the type/fix Bug fix label Oct 16, 2023

chenzl25 added component/connector and removed ci/run-java-binding-tests ci/run-java-connector-node-integration-tests labels Oct 16, 2023

chenzl25 added ci/run-java-binding-tests ci/run-java-connector-node-integration-tests component/ci and removed ci/run-java-binding-tests labels Oct 16, 2023

Merge branch 'main' into dylan/fix_java_binding_TLS_allocation_failure

7cdf801

chenzl25 force-pushed the dylan/fix_java_binding_TLS_allocation_failure branch from 6da334b to 7cdf801 Compare October 17, 2023 06:23

chenzl25 and others added 3 commits October 17, 2023 14:23

Merge remote-tracking branch 'origin/main' into dylan/fix_java_bindin…

9cf03ff

…g_TLS_allocation_failure

add disable_initial_exec_tls feature to common

2292ba0

Fix "cargo-hakari"

961c4d0

github-actions bot requested a review from a team as a code owner October 17, 2023 06:30

chenzl25 added 2 commits October 17, 2023 14:40

fix

7612fe1

Revert "fix"

b004840

This reverts commit fe6820b.

chenzl25 requested review from wenym1, yuhao-su, fuyufjh and TennyZhuang October 17, 2023 07:08

BugenZhao reviewed Oct 17, 2023

View reviewed changes

github-actions bot removed the component/ci label Oct 17, 2023

fix hakari

6dd7710

chenzl25 requested review from BugenZhao and xxchan October 17, 2023 09:00

fuyufjh approved these changes Oct 17, 2023

View reviewed changes

wenym1 approved these changes Oct 17, 2023

View reviewed changes

yuhao-su approved these changes Oct 17, 2023

View reviewed changes

move common/heap_profiling to a separate crate

f8f7831

fix

3c037ff

chenzl25 added this pull request to the merge queue Oct 17, 2023

chenzl25 added the need-cherry-pick-v1.3 label Oct 17, 2023

Merged via the queue into main with commit 34ec260 Oct 17, 2023
16 checks passed

chenzl25 deleted the dylan/fix_java_binding_TLS_allocation_failure branch October 17, 2023 10:39

github-actions bot mentioned this pull request Oct 17, 2023

cherrypick fix(java_binding): fix java binding TLS allocation failure (#12862) to branch v1.3-rc #12917

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(java_binding): fix java binding TLS allocation failure #12862

fix(java_binding): fix java binding TLS allocation failure #12862

chenzl25 commented Oct 16, 2023 •

edited

Loading

codecov bot commented Oct 16, 2023 •

edited

Loading

StrikeW commented Oct 17, 2023 •

edited

Loading

chenzl25 commented Oct 17, 2023 •

edited

Loading

fuyufjh commented Oct 17, 2023

BugenZhao Oct 17, 2023

yuhao-su Oct 17, 2023

xxchan Oct 17, 2023

chenzl25 Oct 17, 2023

xxchan Oct 17, 2023

chenzl25 Oct 17, 2023

BugenZhao Oct 17, 2023 •

edited

Loading

yuhao-su Oct 17, 2023

xxchan Oct 17, 2023

This comment was marked as resolved.

chenzl25 commented Oct 17, 2023

fuyufjh left a comment

xxchan commented Oct 17, 2023

yuhao-su commented Oct 17, 2023

StrikeW commented Oct 17, 2023

chenzl25 commented Oct 17, 2023

StrikeW commented Oct 17, 2023

fix(java_binding): fix java binding TLS allocation failure #12862

fix(java_binding): fix java binding TLS allocation failure #12862

Conversation

chenzl25 commented Oct 16, 2023 • edited Loading

What's changed and what's your intention?

Checklist

Documentation

Release note

codecov bot commented Oct 16, 2023 • edited Loading

Codecov Report

StrikeW commented Oct 17, 2023 • edited Loading

chenzl25 commented Oct 17, 2023 • edited Loading

fuyufjh commented Oct 17, 2023

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

BugenZhao Oct 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

chenzl25 commented Oct 17, 2023

fuyufjh left a comment

Choose a reason for hiding this comment

xxchan commented Oct 17, 2023

yuhao-su commented Oct 17, 2023

StrikeW commented Oct 17, 2023

chenzl25 commented Oct 17, 2023

StrikeW commented Oct 17, 2023

chenzl25 commented Oct 16, 2023 •

edited

Loading

codecov bot commented Oct 16, 2023 •

edited

Loading

StrikeW commented Oct 17, 2023 •

edited

Loading

chenzl25 commented Oct 17, 2023 •

edited

Loading

BugenZhao Oct 17, 2023 •

edited

Loading