Skip to content

Increase jemalloc aarch64 page size limit (#5244) #6831

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

Merged
merged 4 commits into from
Jan 30, 2025

Conversation

ph03
Copy link
Contributor

@ph03 ph03 commented Jan 21, 2025

Issue Addressed

#5244

Proposed Changes

Pass JEMALLOC_SYS_WITH_LG_PAGE=16 env to aarch64 cross-compilation to support systems with up to 64-KiB page sizes. This is backwards-compatible for the current (most usual) 4-KiB systems.

Pass JEMALLOC_SYS_WITH_LG_PAGE=16 to aarch64
cross-compilation to support systems with
up to 64-KiB page sizes.
@CLAassistant
Copy link

CLAassistant commented Jan 21, 2025

CLA assistant check
All committers have signed the CLA.

@michaelsproul michaelsproul added under-review A reviewer has only partially completed a review. waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Jan 23, 2025
also add JEMALLOC_SYS_WITH_LG_PAGE to `build-lcli-aarch64`
target
@michaelsproul michaelsproul added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. under-review A reviewer has only partially completed a review. labels Jan 29, 2025
@michaelsproul michaelsproul added the backwards-incompat Backwards-incompatible API change label Jan 30, 2025
Copy link
Member

@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

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

Looks good.

I added the page size info to lighthouse --version so we can confirm when the setting is taking effect.

I had issues with Cargo not detecting the setting of the env var, like:

  1. Build without env var
  2. Build with env var (no-op, but should rebuild)

I think this is maybe just something to be aware of. A cargo clean between 1 and 2 will sort it out.

@michaelsproul michaelsproul added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Jan 30, 2025
@michaelsproul
Copy link
Member

Added the backwards-incompat label too so that we remember to mention this in the release notes.

mergify bot added a commit that referenced this pull request Jan 30, 2025
@mergify mergify bot merged commit d297d08 into sigp:unstable Jan 30, 2025
31 checks passed
@michaelsproul
Copy link
Member

michaelsproul commented Jan 30, 2025

Some benchmarks on my Mac showed an insignificant (~1%) difference between 16K pages (default on macOS) and 64K pages, when it came to CPU performance of lcli skip-slots.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backwards-incompat Backwards-incompatible API change ready-for-merge This PR is ready to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants