Skip to content

Comments

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

Merged
mergify[bot] merged 4 commits intosigp:unstablefrom
janickm:set-jemalloc-lg-page
Jan 30, 2025
Merged

Increase jemalloc aarch64 page size limit (#5244)#6831
mergify[bot] merged 4 commits intosigp:unstablefrom
janickm:set-jemalloc-lg-page

Conversation

@janickm
Copy link
Contributor

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