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

chore: Use mimalloc as the default allocator #7059

Closed
wants to merge 5 commits into from
Closed

Conversation

jfecher
Copy link
Contributor

@jfecher jfecher commented Jan 14, 2025

Description

Problem*

Resolves

Summary*

Testing to see if this improves performance

Additional Context

Documentation*

Check one:

  • No documentation needed.
  • Documentation included in this PR.
  • [For Experimental Features] Documentation to be submitted in a separate PR.

PR Checklist*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

Copy link
Contributor

github-actions bot commented Jan 14, 2025

Compilation Memory Report

Program Peak Memory %
keccak256 107.15K 100%
workspace 107.15K 100%
regression_4709 107.15K 100%
ram_blowup_regression 107.15K 100%
rollup-base-public 107.15K 100%
rollup-base-private 107.15K 100%
private-kernel-tail 107.15K 100%
private-kernel-reset 107.15K 100%
private-kernel-inner 107.15K 100%

Copy link
Contributor

github-actions bot commented Jan 14, 2025

Execution Memory Report

Program Peak Memory %
keccak256 107.15K 100%
workspace 107.15K 100%
regression_4709 107.15K 100%
ram_blowup_regression 107.15K 100%
rollup-base-public 107.15K 100%
rollup-base-private 107.15K 100%
private-kernel-tail 107.15K 100%
private-kernel-reset 107.15K 100%
private-kernel-inner 107.15K 100%

Copy link
Contributor

github-actions bot commented Jan 14, 2025

Compilation Report

Program Compilation Time %
sha256_regression 1.030s -19%
regression_4709 0.756s -5%
ram_blowup_regression 15.200s -11%
rollup-root 3.206s -4%
rollup-merge 1.786s -3%
rollup-block-merge 3.142s -6%
rollup-base-public 33.120s -9%
rollup-base-private 10.800s -14%
private-kernel-tail 0.869s -11%
private-kernel-reset 5.306s -16%
private-kernel-inner 1.902s -5%

Copy link
Contributor

github-actions bot commented Jan 14, 2025

Execution Report

Program Execution Time %
sha256_regression 0.051s -2%
regression_4709 0.003s 200%
ram_blowup_regression 0.610s 0%
rollup-root 0.105s 0%
rollup-merge 0.007s 16%
rollup-block-merge 0.105s 0%
rollup-base-public 1.218s -1%
rollup-base-private 0.455s 0%
private-kernel-tail 0.019s 0%
private-kernel-reset 0.315s 0%
private-kernel-inner 0.068s 0%

@jfecher
Copy link
Contributor Author

jfecher commented Jan 14, 2025

Some good compilation time improvements here for such a small change but the memory reports are broken

@jfecher jfecher requested a review from a team January 14, 2025 15:21
@ludamad
Copy link
Collaborator

ludamad commented Jan 14, 2025

Interesting, thought rust had quite a good allocator

@aakoshh
Copy link
Contributor

aakoshh commented Jan 14, 2025

FWIW I tried the profiling I do with Instruments in #7053 and it seems to have bombed (I'm on Mac):

Screenshot 2025-01-14 at 15 32 54

Or I'm not sure what happens exactly, usually it runs for minutes, and the 12GB is also not something I have seen before.

If I un e.g. info I do see output though 🤷
Maybe the profiler could not attach itself and the compilation stayed fast, but the reported memory completely bogus.

But no, even that's not true, I have stack trace:
Screenshot 2025-01-14 at 15 42 50

It just shows way more memory than expected.

@TomAFrench
Copy link
Member

Yeah, heaptrack reports that it doesn't work with custom allocators so we may need to replace this tooling for something else if we go forwards with this.

@jfecher
Copy link
Contributor Author

jfecher commented Jan 14, 2025

@ludamad Rust just uses the default system allocator

@jfecher
Copy link
Contributor Author

jfecher commented Jan 14, 2025

Looks like valgrind's massif tool isn't working either

@jfecher
Copy link
Contributor Author

jfecher commented Jan 14, 2025

Profiling with MIMALLOC_SHOW_STATS=1 I also see the 12GB peak usage. Switching this PR to try out TCMalloc instead

@jfecher
Copy link
Contributor Author

jfecher commented Jan 14, 2025

This could be something to continue looking into for the future but I currently can't seem to profile memory usage of jemalloc, and any alternate allocator in general would break our CI so I'm closing this for now despite the possible performance gains.

@jfecher jfecher closed this Jan 14, 2025
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.

4 participants