-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Conversation
run |
@ordian thanks I just realize it, I was running test on a deprecated hash. |
do you need a macos pipeline here? |
yes, I temporarily put a 'test-build-darwin' as single test target, strange it doesn't launch on the last commit. |
@ordian it ends up that the problem was indeed with
it needed
and in code it also needed the target_os check. |
There was a problem hiding this 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
?
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). |
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. |
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). |
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. |
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). |
.gitlab-ci.yml
Outdated
@@ -35,6 +35,18 @@ variables: | |||
- export VERSION | |||
- echo "Version = ${VERSION}" | |||
|
|||
test-macos: |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I switch it off, for reference https://gitlab.parity.io/parity/parity-ethereum/-/jobs/118672
There was a problem hiding this 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.
@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). |
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? |
* 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.
* 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.
* 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.
Macos heapsize force jemalloc (openethereum#10234)
* 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 <5chdn@users.noreply.github.com>
* 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.
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).