-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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(es/minifier): Speed up merge_sequences_in_exprs
by caching computation
#9843
perf(es/minifier): Speed up merge_sequences_in_exprs
by caching computation
#9843
Conversation
🦋 Changeset detectedLatest commit: 75045a4 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI failed, and I verified locally that CI failure does not happen on the main branch.
I love the idea behind this PR. I think it's fine to simply update test cases of the swc
crate in this case, considering the performance difference and the amount of difference in outputs. Can you update or fix them? You can do either of them.
merge_sequences_in_exprs
by caching computationmerge_sequences_in_exprs
by caching computation
CodSpeed Performance ReportMerging #9843 will degrade performances by 3.33%Comparing Summary
Benchmarks breakdown
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Problem
In https://github.com/chenjiahan/rspack-swc-minify-test:
Rspack + SWC minify: 15.89 s
Rspack + esbuild minify: 1.74 s
This is caused by
merge_sequences_in_exprs
which tries to merge every pair of exprs with time complexity of O(n^2). In the given example, a vec of 5003 exprs (react component declarations and other declarations) are passed to this function.From the profiling result below we can see
visit_children_with
takes a lot of times, which is called byIdentUsageFinder
andidents_used_by
to check whether an ident is used in an ast node. However, in the O(n^2) loops, this part of the result is heavily recalculated.Example:
Solution
To be honest, I don't come up with a better algorithm to reduce time complexity or pruning. But caching part of the computation does work very well in this bad case. Anyway, I'm proposing this pr first.
Result
Before: 10128ms
After: 1013ms(~10x)