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: reduce large pre-allocations for JavascriptParser::new #7286

Merged
merged 1 commit into from
Jul 23, 2024

Conversation

h-a-n-a
Copy link
Collaborator

@h-a-n-a h-a-n-a commented Jul 23, 2024

Summary

AsyncDependenciesBlock is of the size of 208 bytes. It was pre-allocated for the capacity of 256:

size_of::<AsyncDependenciesBlock>() = 208

Taking a project with 10000 modules. It will at least allocate (and destroy) the memory of 256 * 208 * 10000 bytes (507Mbs), causing a memory increment and decrement back and forth.

Since the layout AsyncDependenciesBlock is quite compat, two approaches remain:

  • Change Vec<AsyncDependenciesBlock> to Vec<Box<AsyncDependenciesBlock>>
  • Change initial capacity of the Vec to a smaller value

The first solution is to reduce the memory usage from 256 * 208 to 256 * 24, giving us a significant memory reduce, however with some potential overhead for sending the value to the heap.
With the second capacity change from 256 to 64.

I also changed the initial capacity of other vecs, although it does not(and should not) have much memory cost difference.

Performance for big projects

The performance difference between these two changes of our internal project of 38134 modules remain unnoticeable. It's around 30 seconds for each build on my computer (Apple M2 Max, 64 GB).

Memory cost

The memory cost of this is a constant value for each module. With the difference number of modules, it results in different amount of allocations created and destroyed. Taking 10000 modules case as an example, we can notice the difference of memory being created and destroyed during each build:

Before:
img_v3_02d2_979ee573-67e2-4211-8105-58e1a8a4d0eg

After:
img_v3_02d2_a89f6588-d5ab-49b0-8877-f80ac9e4e88g

It's 12.5% improvement of the total memory being allocated.

Clippy complaints of Vec<Box<T>> where T: Sized

Clippy is complaining that T of Vec<T> was already allocated on the heap. Since it's a huge struct with that much of pre-allocation, we can workaround this as it was said in the detail page: rust-lang/rust-clippy#3530

Checklist

  • Tests updated (or not required).
  • Documentation updated (or not required).

@github-actions github-actions bot added release: performance release: performance related release(mr only) team The issue/pr is created by the member of Rspack. labels Jul 23, 2024
@h-a-n-a
Copy link
Collaborator Author

h-a-n-a commented Jul 23, 2024

!bench

@rspack-bot
Copy link

rspack-bot commented Jul 23, 2024

📝 Benchmark detail: Open

Name Base (2024-07-23 26d1cb6) Current Change
10000_development-mode exec 2.25 s ± 22 ms 2.24 s ± 13 ms -0.25 %
10000_development-mode_hmr exec 699 ms ± 10 ms 697 ms ± 7.3 ms -0.36 %
10000_production-mode exec 2.84 s ± 38 ms 2.84 s ± 25 ms -0.20 %
arco-pro_development-mode exec 1.91 s ± 73 ms 1.91 s ± 67 ms 0.00 %
arco-pro_development-mode_hmr exec 434 ms ± 0.95 ms 434 ms ± 0.9 ms -0.08 %
arco-pro_production-mode exec 3.47 s ± 109 ms 3.51 s ± 87 ms 1.14 %
threejs_development-mode_10x exec 1.77 s ± 20 ms 1.79 s ± 21 ms 1.44 %
threejs_development-mode_10x_hmr exec 865 ms ± 6.6 ms 884 ms ± 6 ms 2.21 %
threejs_production-mode_10x exec 5.74 s ± 25 ms 5.8 s ± 40 ms 1.00 %

@h-a-n-a
Copy link
Collaborator Author

h-a-n-a commented Jul 23, 2024

!bench

@rspack-bot
Copy link

rspack-bot commented Jul 23, 2024

📝 Benchmark detail: Open

Name Base (2024-07-23 26d1cb6) Current Change
10000_development-mode exec 2.25 s ± 22 ms 2.24 s ± 20 ms -0.03 %
10000_development-mode_hmr exec 699 ms ± 10 ms 693 ms ± 6.8 ms -0.84 %
10000_production-mode exec 2.84 s ± 38 ms 2.83 s ± 23 ms -0.47 %
arco-pro_development-mode exec 1.91 s ± 73 ms 1.94 s ± 39 ms 1.70 %
arco-pro_development-mode_hmr exec 434 ms ± 0.95 ms 435 ms ± 1.3 ms 0.11 %
arco-pro_production-mode exec 3.47 s ± 109 ms 3.48 s ± 68 ms 0.23 %
threejs_development-mode_10x exec 1.77 s ± 20 ms 1.79 s ± 25 ms 1.31 %
threejs_development-mode_10x_hmr exec 865 ms ± 6.6 ms 882 ms ± 7.1 ms 1.99 %
threejs_production-mode_10x exec 5.74 s ± 25 ms 5.79 s ± 45 ms 0.71 %

@h-a-n-a h-a-n-a changed the title perf: reduce allocation for JavascriptParser::new perf: reduce large pre-allocations for JavascriptParser::new Jul 23, 2024
@h-a-n-a h-a-n-a marked this pull request as ready for review July 23, 2024 15:10
@h-a-n-a h-a-n-a enabled auto-merge (squash) July 23, 2024 15:10
@h-a-n-a h-a-n-a requested review from JSerFeng and ahabhgk July 23, 2024 15:10
@h-a-n-a h-a-n-a merged commit f6c8932 into main Jul 23, 2024
46 checks passed
@h-a-n-a h-a-n-a deleted the perf-javascript-parser branch July 23, 2024 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release: performance release: performance related release(mr only) team The issue/pr is created by the member of Rspack.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants