Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Macos heapsize force jemalloc #10234

Merged
merged 13 commits into from
Jan 29, 2019
Merged

Macos heapsize force jemalloc #10234

merged 13 commits into from
Jan 29, 2019

Conversation

cheme
Copy link
Contributor

@cheme cheme commented Jan 22, 2019

This PR is a temporary fix for heapsize issue on mac.

It uses a fork of heapsize that uses jemallocator crate. The fork is currently on my repo, maybe creating paritytech/heapsize could be an idea, but it conflicts with target that is having
paritytech/parity-common#93 merged and remove heapsize from parity-ethereum (needs an other PR merged and crates published so this commit is probably safer regarding current deadlines).

The build pass https://gitlab.parity.io/parity/parity-ethereum/-/jobs/118084 on a modified ci config (I restore the original config in latest commit).

@cheme cheme added the A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. label Jan 22, 2019
@5chdn 5chdn added this to the 2.4 milestone Jan 23, 2019
@5chdn 5chdn added the M0-build 🏗 Building and build system. label Jan 23, 2019
@ordian
Copy link
Collaborator

ordian commented Jan 23, 2019

run cargo update -p heapsize to update Cargo.lock (it uses old heapsize commit 4dcb303d)

@cheme
Copy link
Contributor Author

cheme commented Jan 23, 2019

@ordian thanks I just realize it, I was running test on a deprecated hash.

@5chdn
Copy link
Contributor

5chdn commented Jan 23, 2019

do you need a macos pipeline here?

@cheme
Copy link
Contributor Author

cheme commented Jan 23, 2019

yes, I temporarily put a 'test-build-darwin' as single test target, strange it doesn't launch on the last commit.
Edit : no seems like it is pending.

@cheme
Copy link
Contributor Author

cheme commented Jan 23, 2019

@ordian it ends up that the problem was indeed with

[target.'cfg(macos)'.dependencies]

it needed

[target.'cfg(target_os = "macos")'.dependencies]

and in code it also needed the target_os check.

@cheme cheme added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Jan 24, 2019
Copy link
Collaborator

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

LGTM, but this is only a temporary fix right?

We need create an issue to fix this so we don't forget but I guess this is ongoing in parity-common?

@cheme
Copy link
Contributor Author

cheme commented Jan 25, 2019

Yes it is temporary, the ongoing issue in 'parity-common' is paritytech/parity-common#93 it is mainly waiting for review, the actual fix will be master...cheme:rem_heapsize but it requires the crates update from parity-common pr (it has been a long time since I did not update the branch so it is a bit out of sync).

@ordian
Copy link
Collaborator

ordian commented Jan 25, 2019

If we're going to release binaries with this fix, I think we should force jemalloc on every supported platform (not only on macOS). Alternatively, we could use rust 1.31, as suggested by @HCastano.

@cheme
Copy link
Contributor Author

cheme commented Jan 25, 2019

The code will indeed work also for linux. I activated it only for macos to be cautious (and tbh also to make sure we do not forget it is temporary).
Edit : if there is a consensus on applying it on linux to I have no objection.

@cheme
Copy link
Contributor Author

cheme commented Jan 25, 2019

One other thing to consider is that by including linux in the fix, we will avoid the slight performance drop related to the 1.32 allocator switch.
If there is no voice against it, I will add it this afternoon.

@cheme
Copy link
Contributor Author

cheme commented Jan 25, 2019

I am facing an issue (probably some cornercase between proc_macro, patching a crate and changing global alloc) when using the same method on linux, so I won't apply to linux too (using malloc_size_of it still behave properly but I do not use cargo patch).
Edit: my parity-ethereum branch macfix3 seems to circumvent the issue, but it is a bit more intrusive (sets the allocator in parity-ethereum), so I would rather stay with the current state of the pr

@5chdn 5chdn added the A8-looksgood 🦄 Pull request is reviewed well. label Jan 28, 2019
@5chdn 5chdn added B1-patch-beta 🕷🕷 B0-patch-stable 🕷 Pull request should also be back-ported to the stable branch. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 28, 2019
.gitlab-ci.yml Outdated
@@ -35,6 +35,18 @@ variables:
- export VERSION
- echo "Version = ${VERSION}"

test-macos:
Copy link
Contributor

Choose a reason for hiding this comment

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

do we want to keep this?

Copy link
Contributor Author

@cheme cheme Jan 28, 2019

Choose a reason for hiding this comment

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

If we merge the PR : probably not (itwill really slowdown CI of every PR). I activated it last week because in some case with heapsize failures occurs after build and I wanted to assert it was not the case.
If we only use the PR to patch for the build (not sure if it is possible or even a good idea) we can keep it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@tomaka tomaka left a comment

Choose a reason for hiding this comment

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

This hack is actually cleaner than I thought; it didn't come to my mind that jemallocator exposed the usable_size method.

@cheme
Copy link
Contributor Author

cheme commented Jan 28, 2019

@tomaka it actually run the same way as the parity-util-mem does with 'jemalloc-global' feature so any issue with this will be an issue in parity-common/parity-util-mem (except the linux issue from previous comments that I think is related to the use of cargo patch somehow).

@ordian
Copy link
Collaborator

ordian commented Jan 28, 2019

Note, that it only enables jemalloc on macOS and not on linux (which may incur some perf regression). That was my only concern for approving this PR. But since this is a temporary fix, it's fine I guess?

@5chdn 5chdn merged commit a139c6d into openethereum:master Jan 29, 2019
5chdn pushed a commit that referenced this pull request Jan 29, 2019
* Switch to non prefixed malloc_size_of on macos

* Fix

* Testing darwin build

* Fix import

* conflict

* switch heapsize deps commit

* switch heapsize commit

* Rename branch

* Restore gitlab ci to origin

* test for mac

* mac tests?

* Switch of macos CI tests.
5chdn pushed a commit that referenced this pull request Jan 29, 2019
* Switch to non prefixed malloc_size_of on macos

* Fix

* Testing darwin build

* Fix import

* conflict

* switch heapsize deps commit

* switch heapsize commit

* Rename branch

* Restore gitlab ci to origin

* test for mac

* mac tests?

* Switch of macos CI tests.
afck pushed a commit to poanetwork/parity-ethereum that referenced this pull request Jan 29, 2019
* Switch to non prefixed malloc_size_of on macos

* Fix

* Testing darwin build

* Fix import

* conflict

* switch heapsize deps commit

* switch heapsize commit

* Rename branch

* Restore gitlab ci to origin

* test for mac

* mac tests?

* Switch of macos CI tests.
vkomenda added a commit to poanetwork/parity-ethereum that referenced this pull request Jan 29, 2019
5chdn added a commit that referenced this pull request Jan 30, 2019
* Macos heapsize force jemalloc (#10234)

* Switch to non prefixed malloc_size_of on macos

* Fix

* Testing darwin build

* Fix import

* conflict

* switch heapsize deps commit

* switch heapsize commit

* Rename branch

* Restore gitlab ci to origin

* test for mac

* mac tests?

* Switch of macos CI tests.

* Update Cargo.lock

Co-Authored-By: 5chdn <[email protected]>
5chdn added a commit that referenced this pull request Jan 30, 2019
* Switch to non prefixed malloc_size_of on macos

* Fix

* Testing darwin build

* Fix import

* conflict

* switch heapsize deps commit

* switch heapsize commit

* Rename branch

* Restore gitlab ci to origin

* test for mac

* mac tests?

* Switch of macos CI tests.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. B0-patch-stable 🕷 Pull request should also be back-ported to the stable branch. M0-build 🏗 Building and build system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants