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

Jump to bottom

feat: add a feature to enable jemalloc profiling #1940

Merged
merged 6 commits into from
Mar 19, 2020
Merged

feat: add a feature to enable jemalloc profiling #1940

merged 6 commits into from
Mar 19, 2020

Conversation

yangby-cryptape
Copy link
Collaborator

@yangby-cryptape yangby-cryptape commented Feb 12, 2020 •
edited
Loading

  • fix: fix a ci script which check redundant dependencies
  • feat: run a memory tracker thread to record the memory statistics
    • A thread will be started in background to monitor the whole process, and trace the memory.
  • feat: add a feature to enable jemalloc profiling

@yangby-cryptape yangby-cryptape requested review from a team and zhangsoledad and removed request for zhangsoledad February 12, 2020 20:37
@yangby-cryptape yangby-cryptape added the s:hold Status: Put this issue on hold. label Feb 12, 2020
@yangby-cryptape yangby-cryptape force-pushed the jemalloc-profiling branch 2 times, most recently from 2789832 to 5443ba7 Compare February 13, 2020 06:03
doitian
doitian previously approved these changes Feb 14, 2020
View reviewed changes
@yangby-cryptape yangby-cryptape removed the s:hold Status: Put this issue on hold. label Feb 16, 2020
@yangby-cryptape yangby-cryptape requested review from a team, zhangsoledad, driftluo, xxuejie and doitian and removed request for a team, zhangsoledad and driftluo February 16, 2020 04:08
@yangby-cryptape
Copy link
Collaborator Author

I rebase this PR and push the last commit.

@yangby-cryptape yangby-cryptape added the s:waiting-on-reviewers Status: Waiting for Review label Feb 16, 2020
xxuejie
xxuejie reviewed Feb 16, 2020
View reviewed changes
Cargo.toml Show resolved Hide resolved
@doitian doitian requested a review from driftluo February 17, 2020 11:27
jjyr
jjyr previously approved these changes Mar 3, 2020
View reviewed changes
@yangby-cryptape
Copy link
Collaborator Author

The last force-pushed was a mistake.
I checked the diff view of force-pushed, I found only one line changed, I thought I committed a wrong commit.
The fact is: the diff view of force-pushed only display the different between last commit and previous commit.

The last three force-pushed only updated two line:

  • Version of ckb-memory-tracker in Cargo.toml.
  • Version of ckb-memory-tracker in Cargo.lock.

@yangby-cryptape yangby-cryptape requested a review from jjyr March 3, 2020 07:17
@doitian
Copy link
Member

doitian commented Mar 3, 2020 •
edited
Loading

heim-net is using a yanked version of hex. The v0.0.10 fixed this by locking the hex version to v0.4.0.

https://github.com/heim-rs/heim/blob/ba88f7de42f0fb579fe38de596716c009816c768/heim-net/Cargo.toml#L32

@zhangsoledad zhangsoledad mentioned this pull request Mar 11, 2020
Closed
@doitian doitian merged commit c2142a4 into nervosnetwork:develop Mar 19, 2020
@yangby-cryptape yangby-cryptape deleted the jemalloc-profiling branch April 1, 2020 06:14
@yangby-cryptape yangby-cryptape mentioned this pull request Apr 22, 2020
Merged
bors bot added a commit that referenced this pull request Apr 26, 2020
@bors @yangby-cryptape
Merge #2035
48832b3 
2035: fix: remove unsupport configurations in Cargo.toml r=quake,driftluo a=yangby-cryptape

### The Issue

Here is a potential issue which was introduced by #1940.

- Since our `rust-toolchain` is still be `1.41.0`, there will be no errors or warnings when we build the project. (This is why #1940 was merged)

  https://github.com/nervosnetwork/ckb/blob/22829ae85db1643480bd810150f9619250aac92e/rust-toolchain#L1

 - But if you update the `rust-toolchain` to `1.42.0`, when you build the whole project (just execute `make prod`), you will get two warnings:

    > warning: ckb/Cargo.toml: Found `feature = ...` in `target.'cfg(...)'.dependencies`. This key is not supported for selecting dependencies and will not work as expected. Use the [features] section instead: https://doc.rust-lang.org/cargo/reference/features.html
    > warning: ckb/Cargo.toml: Found `feature = ...` in `target.'cfg(...)'.dependencies`. This key is not supported for selecting dependencies and will not work as expected. Use the [features] section instead: https://doc.rust-lang.org/cargo/reference/features.html

~~Although I haven't found any evidences now, I still strongly suspect that it was related to our recent memory leak.~~
**After a lot of tests, I think this issue was not related to our recent memory leak.** But it's still a bug.

### My Solution

I read a lot of issues about cargo in few weeks, I didn't find any official / standard solution to fix it, and I thought they wouldn't fix it in few months. (`build.rs` can't fix it, too)

So, I use a complex and dirty way to fix it, just review the code, please.

#### How can I prove my modification was valid?

- Please check [this example](https://github.com/yangby-cryptape/rust-features-trials/tree/99455ef/features/example-1), I have run it under Debian Buster and Windows 10.

- I have build various kinds of CKB with different features and check a lot logs of them under Debian Buster.

### A Very Important Note

**If any features of a rust bindings (to c library) crate was changed, please do `cargo clean` before build the project.**

Some C libraries wouldn't re-compile by cargo after features changed, for example, `jemalloc-sys`.

Co-authored-by: Boyu Yang <[email protected]>
bors bot added a commit that referenced this pull request Apr 26, 2020
@bors @yangby-cryptape
Merge #2035
5e3abb9 
2035: fix: remove unsupport configurations in Cargo.toml r=quake,driftluo a=yangby-cryptape

### The Issue

Here is a potential issue which was introduced by #1940.

- Since our `rust-toolchain` is still be `1.41.0`, there will be no errors or warnings when we build the project. (This is why #1940 was merged)

  https://github.com/nervosnetwork/ckb/blob/22829ae85db1643480bd810150f9619250aac92e/rust-toolchain#L1

 - But if you update the `rust-toolchain` to `1.42.0`, when you build the whole project (just execute `make prod`), you will get two warnings:

    > warning: ckb/Cargo.toml: Found `feature = ...` in `target.'cfg(...)'.dependencies`. This key is not supported for selecting dependencies and will not work as expected. Use the [features] section instead: https://doc.rust-lang.org/cargo/reference/features.html
    > warning: ckb/Cargo.toml: Found `feature = ...` in `target.'cfg(...)'.dependencies`. This key is not supported for selecting dependencies and will not work as expected. Use the [features] section instead: https://doc.rust-lang.org/cargo/reference/features.html

~~Although I haven't found any evidences now, I still strongly suspect that it was related to our recent memory leak.~~
**After a lot of tests, I think this issue was not related to our recent memory leak.** But it's still a bug.

### My Solution

I read a lot of issues about cargo in few weeks, I didn't find any official / standard solution to fix it, and I thought they wouldn't fix it in few months. (`build.rs` can't fix it, too)

So, I use a complex and dirty way to fix it, just review the code, please.

#### How can I prove my modification was valid?

- Please check [this example](https://github.com/yangby-cryptape/rust-features-trials/tree/99455ef/features/example-1), I have run it under Debian Buster and Windows 10.

- I have build various kinds of CKB with different features and check a lot logs of them under Debian Buster.

### A Very Important Note

**If any features of a rust bindings (to c library) crate was changed, please do `cargo clean` before build the project.**

Some C libraries wouldn't re-compile by cargo after features changed, for example, `jemalloc-sys`.

Co-authored-by: Boyu Yang <[email protected]>
bors bot added a commit that referenced this pull request Apr 27, 2020
@bors @yangby-cryptape
Merge #2035
1fa23fe 
2035: fix: remove unsupport configurations in Cargo.toml r=quake,driftluo a=yangby-cryptape

### The Issue

Here is a potential issue which was introduced by #1940.

- Since our `rust-toolchain` is still be `1.41.0`, there will be no errors or warnings when we build the project. (This is why #1940 was merged)

  https://github.com/nervosnetwork/ckb/blob/22829ae85db1643480bd810150f9619250aac92e/rust-toolchain#L1

 - But if you update the `rust-toolchain` to `1.42.0`, when you build the whole project (just execute `make prod`), you will get two warnings:

    > warning: ckb/Cargo.toml: Found `feature = ...` in `target.'cfg(...)'.dependencies`. This key is not supported for selecting dependencies and will not work as expected. Use the [features] section instead: https://doc.rust-lang.org/cargo/reference/features.html
    > warning: ckb/Cargo.toml: Found `feature = ...` in `target.'cfg(...)'.dependencies`. This key is not supported for selecting dependencies and will not work as expected. Use the [features] section instead: https://doc.rust-lang.org/cargo/reference/features.html

~~Although I haven't found any evidences now, I still strongly suspect that it was related to our recent memory leak.~~
**After a lot of tests, I think this issue was not related to our recent memory leak.** But it's still a bug.

### My Solution

I read a lot of issues about cargo in few weeks, I didn't find any official / standard solution to fix it, and I thought they wouldn't fix it in few months. (`build.rs` can't fix it, too)

So, I use a complex and dirty way to fix it, just review the code, please.

#### How can I prove my modification was valid?

- Please check [this example](https://github.com/yangby-cryptape/rust-features-trials/tree/99455ef/features/example-1), I have run it under Debian Buster and Windows 10.

- I have build various kinds of CKB with different features and check a lot logs of them under Debian Buster.

### A Very Important Note

**If any features of a rust bindings (to c library) crate was changed, please do `cargo clean` before build the project.**

Some C libraries wouldn't re-compile by cargo after features changed, for example, `jemalloc-sys`.

Co-authored-by: Boyu Yang <[email protected]>
bors bot added a commit that referenced this pull request Apr 28, 2020
@bors @yangby-cryptape @doitian
Merge #2035
4598ef8 
2035: fix: remove unsupport configurations in Cargo.toml r=quake,driftluo a=yangby-cryptape

### The Issue

Here is a potential issue which was introduced by #1940.

- Since our `rust-toolchain` is still be `1.41.0`, there will be no errors or warnings when we build the project. (This is why #1940 was merged)

  https://github.com/nervosnetwork/ckb/blob/22829ae85db1643480bd810150f9619250aac92e/rust-toolchain#L1

 - But if you update the `rust-toolchain` to `1.42.0`, when you build the whole project (just execute `make prod`), you will get two warnings:

    > warning: ckb/Cargo.toml: Found `feature = ...` in `target.'cfg(...)'.dependencies`. This key is not supported for selecting dependencies and will not work as expected. Use the [features] section instead: https://doc.rust-lang.org/cargo/reference/features.html
    > warning: ckb/Cargo.toml: Found `feature = ...` in `target.'cfg(...)'.dependencies`. This key is not supported for selecting dependencies and will not work as expected. Use the [features] section instead: https://doc.rust-lang.org/cargo/reference/features.html

~~Although I haven't found any evidences now, I still strongly suspect that it was related to our recent memory leak.~~
**After a lot of tests, I think this issue was not related to our recent memory leak.** But it's still a bug.

### My Solution

I read a lot of issues about cargo in few weeks, I didn't find any official / standard solution to fix it, and I thought they wouldn't fix it in few months. (`build.rs` can't fix it, too)

So, I use a complex and dirty way to fix it, just review the code, please.

#### How can I prove my modification was valid?

- Please check [this example](https://github.com/yangby-cryptape/rust-features-trials/tree/99455ef/features/example-1), I have run it under Debian Buster and Windows 10.

- I have build various kinds of CKB with different features and check a lot logs of them under Debian Buster.

### A Very Important Note

**If any features of a rust bindings (to c library) crate was changed, please do `cargo clean` before build the project.**

Some C libraries wouldn't re-compile by cargo after features changed, for example, `jemalloc-sys`.

Co-authored-by: Boyu Yang <[email protected]>
Co-authored-by: ian <[email protected]>
bors bot added a commit that referenced this pull request Apr 29, 2020
@bors @yangby-cryptape
Merge #2035
c4c4157 
2035: fix: remove unsupport configurations in Cargo.toml r=doitian,TheWaWaR a=yangby-cryptape

### The Issue

Here is a potential issue which was introduced by #1940.

- Since our `rust-toolchain` is still be `1.41.0`, there will be no errors or warnings when we build the project. (This is why #1940 was merged)

  https://github.com/nervosnetwork/ckb/blob/22829ae85db1643480bd810150f9619250aac92e/rust-toolchain#L1

 - But if you update the `rust-toolchain` to `1.42.0`, when you build the whole project (just execute `make prod`), you will get two warnings:

    > warning: ckb/Cargo.toml: Found `feature = ...` in `target.'cfg(...)'.dependencies`. This key is not supported for selecting dependencies and will not work as expected. Use the [features] section instead: https://doc.rust-lang.org/cargo/reference/features.html
    > warning: ckb/Cargo.toml: Found `feature = ...` in `target.'cfg(...)'.dependencies`. This key is not supported for selecting dependencies and will not work as expected. Use the [features] section instead: https://doc.rust-lang.org/cargo/reference/features.html

~~Although I haven't found any evidences now, I still strongly suspect that it was related to our recent memory leak.~~
**After a lot of tests, I think this issue was not related to our recent memory leak.** But it's still a bug.

### My Solution

I read a lot of issues about cargo in few weeks, I didn't find any official / standard solution to fix it, and I thought they wouldn't fix it in few months. (`build.rs` can't fix it, too)

So, I use a complex and dirty way to fix it, just review the code, please.

#### How can I prove my modification was valid?

- Please check [this example](https://github.com/yangby-cryptape/rust-features-trials/tree/99455ef/features/example-1), I have run it under Debian Buster and Windows 10.

- I have build various kinds of CKB with different features and check a lot logs of them under Debian Buster.

### A Very Important Note

**If any features of a rust bindings (to c library) crate was changed, please do `cargo clean` before build the project.**

Some C libraries wouldn't re-compile by cargo after features changed, for example, `jemalloc-sys`.

Co-authored-by: Boyu Yang <[email protected]>
@bowenwang1996 bowenwang1996 mentioned this pull request Sep 26, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xxuejie xxuejie xxuejie left review comments

@quake quake quake approved these changes

@doitian doitian doitian approved these changes

@driftluo driftluo Awaiting requested review from driftluo

@jjyr jjyr Awaiting requested review from jjyr

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@yangby-cryptape @doitian @quake @xxuejie @jjyr @driftluo