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

perf: Use faster malloc #188

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

perf: Use faster malloc #188

wants to merge 1 commit into from

Conversation

kdy1
Copy link

@kdy1 kdy1 commented Nov 11, 2023

Hi. I'm the creator of SWC, and I'm working to improve performance.

swc_malloc is a utility crate that configures global malloc with the best one for each platform. I made it mainly for benchmarks, but I found it useful so I'm also using it for the official node bindings (including extra bindings - CSS/HTML/XML)

If you want, you can store it inline, but with this crate you don't need to maintain it.

@anonrig
Copy link
Member

anonrig commented Nov 11, 2023

Can you share your local benchmark results?

@kdy1
Copy link
Author

kdy1 commented Nov 11, 2023

I didn't post it because the benchmark was too flaky, but there you go.

image

@kdy1
Copy link
Author

kdy1 commented Nov 13, 2023

Can you try profiling from your PC?

Cargo.toml Outdated Show resolved Hide resolved
@kdy1 kdy1 force-pushed the perf-1-malloc branch 3 times, most recently from 57168b8 to 9886f5e Compare November 22, 2023 05:47
@polarathene
Copy link

polarathene commented Nov 23, 2023

swc_malloc is a utility crate that configures global malloc with the best one for each platform.

Do you have any information that backs up that "best" claim for reference?

Presently it looks like swc_malloc only has two allocators that'd be selected? mimalloc and jemalloc (Cargo.toml + conditionals for each in lib.rs).

You use mimalloc for anything not on linux, and jemalloc for linux (but only x86_64 + aarch64 archs on -gnu target), anything else will fall back to the default? So that will not be the best for -musl targets which tend to perform poorly (especially on multi-threaded contexts) with the default allocator.


You may want to evaluate snmalloc which may be worthwhile.

Keep in mind that the most appropriate allocator choice really depends on the workload context.

  • So pushing swc_malloc elsewhere may not make the "best" allocator choice for other projects, the effectiveness could vary on which one is actually best for a projects needs?
  • Given the prefix, would it be safe to say that the choice of allocator is subject to change, will it be a breaking semver change or implicitly forced upon downstreams where regressions may occur?

Copy link

codecov bot commented Nov 24, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c62a631) 92.65% compared to head (e001251) 92.72%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #188      +/-   ##
==========================================
+ Coverage   92.65%   92.72%   +0.07%     
==========================================
  Files          57       57              
  Lines        2847     2847              
==========================================
+ Hits         2638     2640       +2     
+ Misses        209      207       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kdy1
Copy link
Author

kdy1 commented Nov 24, 2023

Feel free to inline it if you want

@polarathene
Copy link

polarathene commented Nov 24, 2023

As @kdy1 does not seem interested in responding to the raised concerns, it is probably not worthwhile merging the PR.

  • Claims of allocator selection being best lack evidence / context and may not be applicable to other projects. No clear advantage demonstrated for paquet?
  • Allocator selection excludes either for -musl targets, using the default which performs poorly in multi-threading.
  • Allocator selection may be subject to change implicitly, neither is explicit selection control available.

@kdy1
Copy link
Author

kdy1 commented Nov 24, 2023

Sorry, I'm too lazy to explain it to you.

@kdy1
Copy link
Author

kdy1 commented Nov 24, 2023

If you want description, see the git history of the crate.

It's recently renamed, so you may need to use github UI or you should have a bit of git skills.

@polarathene
Copy link

polarathene commented Nov 24, 2023

If you want description, see the git history of the crate.

I did, the original crate introduced the allocator here with both jemalloc and mimalloc (specific commit from PR branch).

Like with other commits and PRs related to the allocator history, there is very little information beyond some bench result snippets shared in PRs. These provide minimal context and instill no confidence in addressing the concerns I raised about merging your PR here.


I'm too lazy to explain it to you.

Being lazy to respond to these questions is ok 👍 But by not doing it should discourage @KSXGitHub from approving this PR.

  • You're aware that just because you had an improvement in your bench, it does not mean it will be "best" for all projects? All you have to support your choice is a few lines of benches related to your project?
  • You're aware that your crates config patterns is non-exhaustive? The common musl targets are excluded intentionally (without any reasoning?)
  • You're aware that you may change the allocators in future, or adjust which targets qualify for a specific allocator, yet won't express if that'd be treated as a breaking change semver bump, or if you'd provide any explicit control of allocator selection?

You are promoting adoption of something unreliable, with very little evidence to support it. @KSXGitHub would be better off explicitly managing the global allocator themselves (should they see value in switching the allocator wholesale, or configuring multiple based on target).

@kdy1
Copy link
Author

kdy1 commented Dec 2, 2023

xxx_malloc crate is important for multi-crate projects because you can do extern crate xxx_malloc from benchmark files. It does not replace the default malloc if there's no extern crate. If you try to inline it into a final binary crate, configuring malloc for benchmarking becomes very cumbersome.

#[cfg.target(xxx).dependencies]

in Cargo.toml of each crate is required.


One more problem is that, over time the build breaks. That's why I simply use swc_malloc (formerly swc_node_base) from my other private projects. I'll maintain SWC anyway - so I don't need to update my side project just because of the build environment change.


About musl, those are excluded because musl builds are very slow in GitHub action.


Sorry for being rude. I had too many tasks at that time.

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