Skip to content

Replace malloc with mimalloc #142

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

Closed
ammarahm-ed opened this issue Nov 26, 2023 · 20 comments
Closed

Replace malloc with mimalloc #142

ammarahm-ed opened this issue Nov 26, 2023 · 20 comments

Comments

@ammarahm-ed
Copy link
Contributor

ammarahm-ed commented Nov 26, 2023

Using mimalloc instead of malloc improves performance significantly.

Pros: Improved performance with a small change

Cons: It will add an external dependency and slightly increase size of QuickJS binaries.

It has been previously added here: openwebf/quickjs#30

Here's the benchmark I ran after replacing:

Benchmark Master Mimalloc Percent Improvement
DeltaBlue 605 702 15.99%
Crypto 863 906 4.98%
RayTrace 357 963 169.33%
EarleyBoyer 682 1579 131.70%
RegExp 149 220 47.65%
Splay 736 1804 145.76%
SplayLatency 3088 4543 47.16%
NavierStokes 1511 1590 5.24%
PdfJS 2016 2949 46.21%
Mandreel 604 656 8.61%
MandreelLatency 4803 5097 6.13%
Gameboy 5191 5644 8.72%
CodeLoad 8015 13343 66.20%
Box2D 1905 2632 38.03%
Typescript 7088 9413 32.75%
Score (version 9) 1430 2060 43.98%

It could also be opt-in by the way.

@saghul
Copy link
Contributor

saghul commented Nov 26, 2023

We should do new benchmarks when considering something like this, FWIW.

@bnoordhuis
Copy link
Contributor

Using a custom allocator is problematic for libquickjs.a due to:

  • risk of symbol conflicts in the embedding program unless the allocator is completely namespaced off

  • allocator mixup bugs (allocating with allocator A, releasing with B); maybe not a problem for quickjs but js_malloc and friends are exposed in quickjs.h

Embedders can replace the default allocator. That's something we could do in the qjs executable, although I'm in no rush to do that right now.

Tangentially, I've spotted several places where quickjs mallocs a struct that mallocs a struct that mallocs a struct. There's likely gains to be had by flattening such allocation trees.

@ammarahm-ed
Copy link
Contributor Author

ammarahm-ed commented Nov 26, 2023

Using a custom allocator is problematic for libquickjs.a due to:

* risk of symbol conflicts in the embedding program unless the allocator is completely namespaced off

* allocator mixup bugs (allocating with allocator A, releasing with B); maybe not a problem for quickjs but js_malloc and friends _are_ exposed in quickjs.h

Embedders can replace the default allocator. That's something we could do in the qjs executable, although I'm in no rush to do that right now.

Tangentially, I've spotted several places where quickjs mallocs a struct that mallocs a struct that mallocs a struct. There's likely gains to be had by flattening such allocation trees.

  1. That is understandable I think, but also solvable if there are some benefits of using a different allocator. I think mimalloc is already namespaced as mi_malloc, mi_free etc.
  2. If someone uses js_malloc even now, I expect them to free with js_free and internally these apis will switch to mimalloc so if someone does end up allocating memory with malloc and then trying to free with js_free, it's on them I would say haha.

There are indeed some optimisations we could do with the default allocators too but i think the difference is just huge when using mimalloc.

@ammarahm-ed
Copy link
Contributor Author

ammarahm-ed commented Nov 26, 2023

@saghul So I did some benchmarks with current quickjs-ng

Benchmark Master Mimalloc Percent Improvement
DeltaBlue 605 702 15.99%
Crypto 863 906 4.98%
RayTrace 357 963 169.33%
EarleyBoyer 682 1579 131.70%
RegExp 149 220 47.65%
Splay 736 1804 145.76%
SplayLatency 3088 4543 47.16%
NavierStokes 1511 1590 5.24%
PdfJS 2016 2949 46.21%
Mandreel 604 656 8.61%
MandreelLatency 4803 5097 6.13%
Gameboy 5191 5644 8.72%
CodeLoad 8015 13343 66.20%
Box2D 1905 2632 38.03%
Typescript 7088 9413 32.75%
Score (version 9) 1430 2060 43.98%

@thecodrr
Copy link

That's a very significant difference. Another allocator worth trying out is jemalloc.

@RobLoach
Copy link
Contributor

Perhaps as an optional dependency? Or an allocator system that lets you add a define to switch which malloc you're using?

@bnoordhuis
Copy link
Contributor

Already the case:

Embedders can replace the default allocator.

@ammarahm-ed
Copy link
Contributor Author

ammarahm-ed commented Nov 29, 2023

Even though you can replace the default allocator yourself which i have done already, making it default positions quickjs at a much better place in terms of performance for everyone and in various benchmarks. That's why i opened up this issue.

@saghul
Copy link
Contributor

saghul commented Nov 29, 2023

While having good benchmark results it would be disingenuous to do that if it turns out it's tricky to use that allocator in all situations.

That said, once I put the docs together, we can have a section where we mention things like this.

@saghul
Copy link
Contributor

saghul commented Jul 16, 2024

@ammarahm-ed I want to benchmark something semi-related to this (using calloc vs malloc + memset to be precise). How did you get the results table from your first post?

@gengjiawen
Copy link

@saghul do you have binary or instructions to build I can test.

I have write a benchmark script in https://github.com/gengjiawen/js-engines-playground/tree/main/benchmark

@saghul
Copy link
Contributor

saghul commented Sep 11, 2024

This is the PR: #519

I'll take a look at your scripts, thank you!

saghul added a commit that referenced this issue Sep 12, 2024
@saghul
Copy link
Contributor

saghul commented Sep 12, 2024

@gengjiawen Out of curiosity, where do those benchmarks come from?

@gengjiawen
Copy link

gengjiawen commented Sep 12, 2024

@gengjiawen Out of curiosity, where do those benchmarks come from?

Use jsuv install engines,than run ‘node benchmark.js’

@saghul
Copy link
Contributor

saghul commented Sep 12, 2024

Sorry I meant that the actual benchmark itself, is it written by V8? What is its canonical source?

@gengjiawen
Copy link

Sorry I meant that the actual benchmark itself, is it written by V8? What is its canonical source?

https://github.com/mozilla/arewefastyet/tree/master/benchmarks/v8-v7

@saghul
Copy link
Contributor

saghul commented Sep 12, 2024

Thank you! I see they are deemed obsolete. Are they still relevant, at least in the case of QuickJS?

@gengjiawen
Copy link

Thank you! I see they are deemed obsolete. Are they still relevant, at least in the case of QuickJS?

Likely so. It’s https://bellard.org/quickjs/bench.html. I think maybe update v9 version.

@saghul
Copy link
Contributor

saghul commented Sep 12, 2024

Gotcha.

@bnoordhuis
Copy link
Contributor

I see they are deemed obsolete. Are they still relevant, at least in the case of QuickJS?

The V8 team stopped using them because they're not really representative of real web apps (fair enough) but they're still useful for benchmarking raw engine prowess.

saghul added a commit that referenced this issue Sep 20, 2024
saghul added a commit that referenced this issue Sep 20, 2024
Some (unscientific) benchmark results:

| Benchmark (Higher scores are better)  | QuickJS           | QuickJS (mimalloc) |
|---------------------------------------|-------------------|--------------------|
| Richards                              | 1217              | 1229               |
| DeltaBlue                             | 1192              | 1297               |
| Crypto                                | 1195              | 1191               |
| RayTrace                              | 1477              | 2186               |
| EarleyBoyer                           | 2441              | 3246               |
| RegExp                                | 275               | 315                |
| Splay                                 | 2461              | 3765               |
| NavierStokes                          | 2156              | 2119               |
| Score                                 | 1318              | 1553               |

Running the V8 benchmark suite (version 7) on an M1 MacBook Pro.

Fixes: #142
@saghul saghul closed this as completed in 6ce2dcc Sep 20, 2024
bluesky950520 pushed a commit to bluesky950520/quickjs that referenced this issue Mar 14, 2025
Some (unscientific) benchmark results:

| Benchmark (Higher scores are better)  | QuickJS           | QuickJS (mimalloc) |
|---------------------------------------|-------------------|--------------------|
| Richards                              | 1217              | 1229               |
| DeltaBlue                             | 1192              | 1297               |
| Crypto                                | 1195              | 1191               |
| RayTrace                              | 1477              | 2186               |
| EarleyBoyer                           | 2441              | 3246               |
| RegExp                                | 275               | 315                |
| Splay                                 | 2461              | 3765               |
| NavierStokes                          | 2156              | 2119               |
| Score                                 | 1318              | 1553               |

Running the V8 benchmark suite (version 7) on an M1 MacBook Pro.

Fixes: quickjs-ng/quickjs#142
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

No branches or pull requests

6 participants