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

Add mimalloc on feature flag #206

Merged
merged 4 commits into from
Jul 6, 2022
Merged

Conversation

passcod
Copy link
Member

@passcod passcod commented Jul 5, 2022

No description provided.

Verified

This commit was signed with the committer’s verified signature.
passcod Félix Saparelli

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Co-authored-by: Jiahao XU <Jiahao_XU@outlook.com>
@passcod
Copy link
Member Author

passcod commented Jul 5, 2022

I think the system linux allocator is pretty good, but perhaps we can use it for musl builds. No strong opinions, though.

@NobodyXu
Copy link
Member

NobodyXu commented Jul 5, 2022

I think the system linux allocator is pretty good, but perhaps we can use it for musl builds. No strong opinions, though.

Musl's malloc is indeed very slow compared to glibc, but honestly, I don't think that is a bottleneck, at least I didn't observe that now.

Maybe when #175 is implemented it might make a difference.

@NobodyXu
Copy link
Member

NobodyXu commented Jul 5, 2022

Looking at the CI build, looks like it doesn't add much to the binary size (0.1M).
So perhaps we can also do that for musl.

BTW, I think we have a couple of C libraries (zstd, libz-ng, xz2, bzip2), not sure whether they use malloc.

@NobodyXu
Copy link
Member

NobodyXu commented Jul 5, 2022

BTW, I think we have a couple of C libraries (zstd, libz-ng, xz2, bzip2), not sure whether they use malloc.

Oh wait, that shouldn't be a problem since mimalloc defines malloc and free, which overrides the libc implementation.

passcod added 2 commits July 6, 2022 23:31

Verified

This commit was signed with the committer’s verified signature.
passcod Félix Saparelli

Verified

This commit was signed with the committer’s verified signature.
passcod Félix Saparelli
@passcod passcod changed the title Use mimalloc on windows Add mimalloc on feature flag Jul 6, 2022
Copy link
Member

@NobodyXu NobodyXu left a comment

Choose a reason for hiding this comment

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

LGTM

@passcod
Copy link
Member Author

passcod commented Jul 6, 2022

Just for fun, ran some basic hyperfine benchmarks on linux (pulling the same archive every run, from a localhost server, not from the internet):

Benchmark 1: binches/x64-gnu-mimalloc --no-confirm --manifest-path . --install-path /tmp cargo-binstall
  Time (mean ± σ):     139.7 ms ±   7.1 ms    [User: 114.3 ms, System: 32.5 ms]
  Range (min … max):   128.5 ms … 161.8 ms    100 runs

Benchmark 2: binches/x64-gnu-sysalloc --no-confirm --manifest-path . --install-path /tmp cargo-binstall
  Time (mean ± σ):     139.1 ms ±   8.8 ms    [User: 114.7 ms, System: 30.5 ms]
  Range (min … max):   127.5 ms … 176.6 ms    100 runs

Benchmark 3: binches/x64-musl-mimalloc --no-confirm --manifest-path . --install-path /tmp cargo-binstall
  Time (mean ± σ):     137.7 ms ±   7.4 ms    [User: 113.2 ms, System: 30.3 ms]
  Range (min … max):   125.6 ms … 164.0 ms    100 runs

Benchmark 4: binches/x64-musl-muslalloc --no-confirm --manifest-path . --install-path /tmp cargo-binstall
  Time (mean ± σ):     134.9 ms ±   7.5 ms    [User: 110.8 ms, System: 27.5 ms]
  Range (min … max):   124.2 ms … 167.1 ms    100 runs

Summary
  'binches/x64-musl-muslalloc --no-confirm --manifest-path . --install-path /tmp cargo-binstall' ran
    1.02 ± 0.08 times faster than 'binches/x64-musl-mimalloc --no-confirm --manifest-path . --install-path /tmp cargo-binstall'
    1.03 ± 0.09 times faster than 'binches/x64-gnu-sysalloc --no-confirm --manifest-path . --install-path /tmp cargo-binstall'
    1.04 ± 0.08 times faster than 'binches/x64-gnu-mimalloc --no-confirm --manifest-path . --install-path /tmp cargo-binstall'

binstall-malloc-stats

@NobodyXu
Copy link
Member

NobodyXu commented Jul 6, 2022

Looks like cargo-binstall does not allocate/deallocate enough to have heap as a bottleneck.

It will also be interesting to run this again once we got #175

@passcod passcod merged commit 3c06c45 into cargo-bins:main Jul 6, 2022
@passcod passcod deleted the mimalloc-on-windows branch July 6, 2022 12:14
@passcod passcod mentioned this pull request Jul 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants