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.

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 polkadot-node-core-pvf-prepare-worker build with jemalloc #1315

Merged
merged 4 commits into from
Aug 31, 2023

Conversation

Morganamilo
Copy link
Contributor

The jemalloc feature on polkadot-node-core-pvf-prepare-worker depended on some feature gated code in polkadot-node-core-pvf-common but there way no way to enable this feature gate.

This commit adds the feature and makes prepare-worker enable it.

The jemalloc feature on polkadot-node-core-pvf-prepare-worker depended
on some feature gated code in polkadot-node-core-pvf-common but there
way no way to enable this feature gate.

This commit adds the feature and makes prepare-worker enable it.
@Morganamilo Morganamilo added the T8-polkadot This PR/Issue is related to/affects the Polkadot network. label Aug 30, 2023
@bkchr bkchr requested a review from mrcnski August 30, 2023 20:23
Copy link
Contributor

@mrcnski mrcnski left a comment

Choose a reason for hiding this comment

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

Thank you! Looks like it was lost when we split up the pvf crates. Few more issues:

  1. The tikv-jemalloc-ctl dependency can be removed from execute-worker/Cargo.toml (note that it's listed twice).
  2. The jemalloc-allocator feature can be added to pvf/Cargo.toml, it should list "polkadot-node-core-pvf-common/jemalloc-allocator".
  3. The jemalloc-allocator feature in polkadot/Cargo.toml should list polkadot-node-core-pvf.
  4. The tikv-jemallocator dep in polkadot/Cargo.toml can be made optional, and added to the Linux-specific list of deps (same as was done in execute-worker/Cargo.toml).
  5. Maybe there should be a CI job that runs cargo check on non-Linux with this feature enabled?

Phew, that's a lot. I can take care of it, I'll just need help with (5). Thanks again!!

@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/3509284

@Morganamilo
Copy link
Contributor Author

Thanks for being more in the know about this and fixing the other issues.

@mrcnski
Copy link
Contributor

mrcnski commented Aug 31, 2023

No problem! It's my fault for missing this. @paritytech/ci would be great to have this job if we don't already have it:

Maybe there should be a CI job that runs cargo check on non-Linux with this feature [jemalloc-allocator] enabled?

@Morganamilo Morganamilo enabled auto-merge (squash) August 31, 2023 13:04
@Morganamilo Morganamilo merged commit aedd280 into master Aug 31, 2023
99 of 101 checks passed
@Morganamilo Morganamilo deleted the morganamilo/jemalloc branch August 31, 2023 15:13
ordian added a commit that referenced this pull request Sep 1, 2023
* master: (25 commits)
  fix typos (#1339)
  Use bandersnatch-vrfs with locked dependencies ref (#1342)
  Bump bs58 from 0.4.0 to 0.5.0 (#1293)
  Contracts: `seal0::balance` should return the free balance (#1254)
  Logs: add extra debug log for negative rep changes (#1205)
  Added short-benchmarks for cumulus (#1183)
  [xcm-emulator] Improve hygiene and clean up (#1301)
  Bump the known_good_semver group with 1 update (#1347)
  Renames API (#1186)
  Rename `polkadot-parachain` to `polkadot-parachain-primitives` (#1334)
  Add README to project root (#1253)
  Add environmental variable to track decoded instructions (#1320)
  Fix polkadot-node-core-pvf-prepare-worker build with jemalloc (#1315)
  Sassafras primitives (#1249)
  Restructure `dispatch` macro related exports (#1162)
  backing: move the min votes threshold to the runtime (#1200)
  Bump zstd from 0.11.2+zstd.1.5.2 to 0.12.4 (#1326)
  Remove `substrate_test_utils::test` (#1321)
  remove disable-runtime-api (#1328)
  [ci] add more jobs for pipeline cancel, cleanup (#1314)
  ...
Daanvdplas pushed a commit that referenced this pull request Sep 11, 2023
* Fix polkadot-node-core-pvf-prepare-worker build with jemalloc

The jemalloc feature on polkadot-node-core-pvf-prepare-worker depended
on some feature gated code in polkadot-node-core-pvf-common but there
way no way to enable this feature gate.

This commit adds the feature and makes prepare-worker enable it.

* More jemalloc-allocator fixes

* Fix jemalloc-allocator feature dep

* Run `zepter format features`

---------

Co-authored-by: Marcin S <marcin@realemail.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T8-polkadot This PR/Issue is related to/affects the Polkadot network.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants