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

Enable jemalloc by default on non windows targets #5995

Merged
merged 2 commits into from
Jun 27, 2024

Conversation

jimmygchen
Copy link
Member

@jimmygchen jimmygchen commented Jun 26, 2024

Issue Addressed

A few discord users that build Lighthouse from source has reported high RAM usage since v5.2.0. @michaelsproul and @chong-he have found that this is due to jemalloc feature being disabled. This is now necessary (on linux) with the introduction of tree-states and should always be enabled.

The Dockerfile doesn't have a default for FEATURES:

lighthouse/Dockerfile

Lines 4 to 7 in f1d88ba

ARG FEATURES
ARG PROFILE=release
ARG CARGO_USE_GIT_CLI=true
ENV FEATURES $FEATURES

From @michaelsproul

it sets it as an argument, which I think means if you don't do --arg FEATURES=jemalloc it will build with FEATURES=""

This PR removes the feature flag and enables jemalloc by default on non windows targets, even when it's not specified via FEATURES environment variable.

@jimmygchen jimmygchen added the ready-for-review The code is ready for review label Jun 26, 2024
@jimmygchen
Copy link
Member Author

Oops looks like this doesn't work in windows, I'll look into it.

@jimmygchen jimmygchen added work-in-progress PR is a work-in-progress and removed ready-for-review The code is ready for review labels Jun 26, 2024
@jimmygchen jimmygchen force-pushed the enable-jemalloc-feature-by-default branch from fd90f8c to 4fab381 Compare June 26, 2024 05:56
@jimmygchen jimmygchen added ready-for-review The code is ready for review backwards-incompat Backwards-incompatible API change low-hanging-fruit Easy to resolve, get it before someone else does! and removed work-in-progress PR is a work-in-progress labels Jun 26, 2024
@michaelsproul
Copy link
Member

With the jemalloc feature deprecated, we should update the allocator_name() function used to show the allocator in lighthouse --version to use the same logic (display "system" on Windows and "jemalloc" on everything else):

fn allocator_name() -> &'static str {
if cfg!(feature = "jemalloc") {
"jemalloc"
} else {
"system"
}
}

@michaelsproul michaelsproul added waiting-on-author The reviewer has suggested changes and awaits thier implementation. v5.3.0 Q3 2024 release with database changes! and removed ready-for-review The code is ready for review labels Jun 27, 2024
@jimmygchen jimmygchen added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Jun 27, 2024
@jimmygchen
Copy link
Member Author

Nice catch, updated!

@jimmygchen jimmygchen added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Jun 27, 2024
@michaelsproul
Copy link
Member

@Mergifyio queue

Copy link

mergify bot commented Jun 27, 2024

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at a910a49

mergify bot added a commit that referenced this pull request Jun 27, 2024
mergify bot added a commit that referenced this pull request Jun 27, 2024
@mergify mergify bot merged commit a910a49 into sigp:unstable Jun 27, 2024
28 checks passed
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 low-hanging-fruit Easy to resolve, get it before someone else does! ready-for-merge This PR is ready to merge. v5.3.0 Q3 2024 release with database changes!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants