Skip to content

Conversation

@jsdt
Copy link
Contributor

@jsdt jsdt commented Mar 14, 2025

Description of Changes

This uses jemalloc in standalone instead of mimalloc, unless the target env is msvc. The main reason is that jemalloc exposes better stats, and has a way to do heap profiling. This PR doesn't set up profiling, but it does add stats on jemalloc's memory usage.

Expected complexity level and risk

  1. The risk here is that there could be an issue on some platforms.

Testing

This works on mac and at least some linux platforms.

@bfops
Copy link
Collaborator

bfops commented Mar 14, 2025

I suspect we should probably figure out if there are any platforms that this won't work on, since we just went through the work to expand our platform support (#2415).

I guess alternatively we could wait to see if any users complain..

@bfops
Copy link
Collaborator

bfops commented Mar 14, 2025

this broadly looks good to me, but I'm not very familiar with different allocator behavior or with the stats part of our code, so I wonder if we can get someone to review who's more confident.

@bfops bfops added the release-any To be landed in any release window label Mar 17, 2025
@Centril
Copy link
Contributor

Centril commented Mar 18, 2025

Could you please run subscription benchmarks before/after as well as the update 10^6 benchmark before/after?

@jsdt
Copy link
Contributor Author

jsdt commented Mar 18, 2025

benchmarks please

@github-actions
Copy link
Contributor

github-actions bot commented Mar 18, 2025

Criterion benchmark results

Error when comparing benchmarks: Couldn't find AWS credentials in environment, credentials file, or IAM role.

Caused by:
Couldn't find AWS credentials in environment, credentials file, or IAM role.

@github-actions
Copy link
Contributor

Callgrind benchmark in progress...

@bfops bfops added backward-compatible release-1.0.1 and removed release-any To be landed in any release window labels Mar 20, 2025
@jsdt jsdt enabled auto-merge March 20, 2025 18:24
Copy link
Collaborator

@bfops bfops 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 as far as I can tell. We think this will almost certainly be an improvement over the system allocator because jemalloc usually is. And @jsdt says he tested it.

@jsdt jsdt added this pull request to the merge queue Mar 20, 2025
Merged via the queue into master with commit a4a2925 Mar 20, 2025
16 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants