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.

Sign up for GitHub

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

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

Merged
merged 10 commits into from
Oct 17, 2023
Merged

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

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
fe6820b
fix
chenzl25 Oct 16, 2023
7cdf801
Merge branch 'main' into dylan/fix_java_binding_TLS_allocation_failure
chenzl25 Oct 16, 2023
9cf03ff
Merge remote-tracking branch 'origin/main' into dylan/fix_java_bindin…
chenzl25 Oct 17, 2023
2292ba0
add disable_initial_exec_tls feature to common
chenzl25 Oct 17, 2023
961c4d0
Fix "cargo-hakari"
chenzl25 Oct 17, 2023
7612fe1
fix
chenzl25 Oct 17, 2023
b004840
Revert "fix"
chenzl25 Oct 17, 2023
6dd7710
fix hakari
chenzl25 Oct 17, 2023
f8f7831
move common/heap_profiling to a separate crate
xxchan Oct 17, 2023
3c037ff
fix
xxchan Oct 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .config/hakari.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ third-party = [
# For some reasons, tikv-jemalloc-sys would be compiled twice if being added into `workspace-hack`
{ name = "tikv-jemalloc-sys", git = "https://github.com/risingwavelabs/jemallocator.git", rev = "64a2d9" },
{ name = "tikv-jemallocator", git = "https://github.com/risingwavelabs/jemallocator.git", rev = "64a2d9" },
{ name = "tikv-jemalloc-ctl", git = "https://github.com/risingwavelabs/jemallocator.git", rev = "64a2d9" },
# These are solely dev-dependencies. Unifying them may slow down build.
{ name = "criterion" },
{ name = "console" },
Expand Down
5 changes: 5 additions & 0 deletions src/common/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,3 +158,8 @@ path = "src/bin/default_config.rs"

[lints]
workspace = true

[features]
default = []
disable_initial_exec_tls = ["tikv-jemalloc-ctl/disable_initial_exec_tls"]
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

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 do think so, let me try. Not quite familiar with hakari before.

Copy link
Member

@BugenZhao BugenZhao Oct 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

This comment was marked as resolved.

Sign in to view


2 changes: 1 addition & 1 deletion src/java_binding/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ normal = ["workspace-hack"]
[dependencies]
jni = "0.21.1"
prost = { workspace = true }
risingwave_common = { workspace = true }
risingwave_common = { workspace = true, features = ["disable_initial_exec_tls"] }
risingwave_jni_core = { workspace = true }
risingwave_pb = { workspace = true }
serde = { version = "1.0", features = ["derive"] }
Expand Down
Loading