Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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

Merged
merged 4 commits into from
Jan 30, 2025

Conversation

ph03
Copy link

@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.

# for free to join this conversation on GitHub. Already have an account? # 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