Skip to content

perf: run lint while constructing nodes#5207

Merged
lukastaegert merged 27 commits intorollup:masterfrom
sapphi-red:perf/run-lint-while-constructing-nodes
Nov 12, 2023
Merged

perf: run lint while constructing nodes#5207
lukastaegert merged 27 commits intorollup:masterfrom
sapphi-red:perf/run-lint-while-constructing-nodes

Conversation

@sapphi-red
Copy link
Contributor

@sapphi-red sapphi-red commented Oct 17, 2023 •
edited by lukastaegert
Loading

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

List any relevant issue numbers:

#5205

Description

This PR implements this idea (#5205 (comment)) of running lint when initialising AST nodes instead of running in SWC side.

Running the benchmark in #5182 (actually I used the esbuild repo instead), I got ~300ms / ~5% improvement on my laptop.

Benchmark results

rollup 3.29.4

# BUILD: 6074ms, 933 MB / 943 MB
## initialize: 0ms, 3.55 kB / 9.81 MB
## generate module graph: 2951ms, 763 MB / 773 MB
- plugin 0 (stdin) - resolveId: 13ms, 2.94 MB / 771 MB
- plugin 0 (stdin) - load: 7ms, 827 kB / 772 MB
generate ast: 626ms, 211 MB / 773 MB
analyze ast: 1435ms, 741 MB / 773 MB
## sort and bind modules: 246ms, 38.5 MB / 812 MB
## mark included statements: 2877ms, 132 MB / 943 MB
treeshaking pass 1: 1029ms, 103 MB / 916 MB
treeshaking pass 2: 474ms, 24 MB / 940 MB
treeshaking pass 3: 178ms, -5.22 MB / 935 MB
treeshaking pass 4: 170ms, 6.52 MB / 942 MB
treeshaking pass 5: 205ms, 321 kB / 942 MB
treeshaking pass 6: 154ms, 14.3 MB / 956 MB
treeshaking pass 7: 136ms, -5.38 MB / 951 MB
treeshaking pass 8: 140ms, 3.45 MB / 954 MB
treeshaking pass 9: 129ms, 1.7 MB / 956 MB
treeshaking pass 10: 131ms, -13.8 MB / 942 MB
treeshaking pass 11: 124ms, 1.11 MB / 943 MB
# GENERATE: 571ms, 106 MB / 1.05 GB
## initialize render: 0ms, 3.46 kB / 945 MB
## generate chunks: 49ms, 3.67 MB / 949 MB
optimize chunks: 1ms, 580 kB / 950 MB
## render chunks: 503ms, 82.9 MB / 1.03 GB
## transform chunks: 18ms, 19.8 MB / 1.05 GB
## generate bundle: 0ms, 1.74 kB / 1.05 GB
# WRITE: 32ms, 19.5 MB / 1.07 GB

rollup 4.1.4

# BUILD: 6398ms, 841 MB / 850 MB
## initialize: 1ms, -636 kB / 8.54 MB
## generate module graph: 3267ms, 663 MB / 672 MB
- plugin 0 (stdin) - resolveId: 14ms, 3.18 MB / 678 MB
- plugin 0 (stdin) - load: 8ms, 882 kB / 674 MB
generate ast: 1025ms, 136 MB / 678 MB
analyze ast: 1405ms, 663 MB / 678 MB
## sort and bind modules: 229ms, 46.4 MB / 718 MB
## mark included statements: 2901ms, 132 MB / 850 MB
treeshaking pass 1: 1060ms, 100 MB / 811 MB
treeshaking pass 2: 496ms, 36.6 MB / 847 MB
treeshaking pass 3: 177ms, -5.25 MB / 842 MB
treeshaking pass 4: 168ms, 6.51 MB / 849 MB
treeshaking pass 5: 195ms, -109 kB / 848 MB
treeshaking pass 6: 153ms, -1.96 MB / 846 MB
treeshaking pass 7: 143ms, 10.9 MB / 857 MB
treeshaking pass 8: 138ms, -12.7 MB / 845 MB
treeshaking pass 9: 117ms, 1.52 MB / 846 MB
treeshaking pass 10: 123ms, 2.79 MB / 849 MB
treeshaking pass 11: 119ms, 1.14 MB / 850 MB
# GENERATE: 565ms, 102 MB / 954 MB
## initialize render: 0ms, 3.46 kB / 852 MB
## generate chunks: 53ms, 2.49 MB / 855 MB
optimize chunks: 1ms, 493 kB / 856 MB
## render chunks: 494ms, 80.2 MB / 935 MB
## transform chunks: 18ms, 19 MB / 954 MB
## generate bundle: 0ms, 1.74 kB / 954 MB
# WRITE: 29ms, 19.9 MB / 974 MB

This PR

# BUILD: 6072ms, 847 MB / 856 MB
## initialize: 0ms, 3.55 kB / 9.24 MB
## generate module graph: 2955ms, 669 MB / 678 MB
- plugin 0 (stdin) - resolveId: 13ms, 1.11 MB / 680 MB
- plugin 0 (stdin) - load: 7ms, 828 kB / 679 MB
generate ast: 710ms, 149 MB / 679 MB
analyze ast: 1385ms, 740 MB / 680 MB
## sort and bind modules: 234ms, 37 MB / 715 MB
## mark included statements: 2882ms, 142 MB / 856 MB
treeshaking pass 1: 1048ms, 99.6 MB / 816 MB
treeshaking pass 2: 489ms, 22 MB / 838 MB
treeshaking pass 3: 182ms, 10.2 MB / 848 MB
treeshaking pass 4: 173ms, 6.71 MB / 855 MB
treeshaking pass 5: 196ms, -509 kB / 855 MB
treeshaking pass 6: 157ms, -1.68 MB / 853 MB
treeshaking pass 7: 141ms, -5.45 MB / 847 MB
treeshaking pass 8: 133ms, 3.45 MB / 851 MB
treeshaking pass 9: 123ms, 1.47 MB / 852 MB
treeshaking pass 10: 115ms, 2.89 MB / 855 MB
treeshaking pass 11: 118ms, 1.2 MB / 856 MB
# GENERATE: 561ms, 101 MB / 959 MB
## initialize render: 0ms, 3.46 kB / 858 MB
## generate chunks: 50ms, 1.75 MB / 860 MB
optimize chunks: 1ms, 380 kB / 862 MB
## render chunks: 495ms, 73 MB / 933 MB
## transform chunks: 16ms, 26.2 MB / 959 MB
## generate bundle: 0ms, 1.74 kB / 959 MB
# WRITE: 32ms, 17.9 MB / 977 MB
Specs of my laptop
  • CPU: Intel Core-i7 1360P
  • Memory: DDR5-4800 32GB
  • OS: Windows 11

References

@vercel
Copy link

vercel bot commented Oct 17, 2023 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
rollup ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 11, 2023 8:07pm

@codecov
Copy link

codecov bot commented Oct 17, 2023 •
edited
Loading

Codecov Report

Merging #5207 (7733df6) into master (52c55bb) will increase coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #5207      +/-   ##
==========================================
+ Coverage   98.82%   98.89%   +0.06%     
==========================================
  Files         231      231              
  Lines        8863     8926      +63     
  Branches     2317     2332      +15     
==========================================
+ Hits         8759     8827      +68     
+ Misses         43       40       -3     
+ Partials       61       59       -2
Files Coverage Δ
src/Module.ts 99.62% <100.00%> (+<0.01%) ⬆️
src/ast/nodes/ArrayPattern.ts 100.00% <ø> (ø)
src/ast/nodes/ArrowFunctionExpression.ts 95.45% <100.00%> (ø)
src/ast/nodes/AssignmentPattern.ts 100.00% <ø> (ø)
src/ast/nodes/CatchClause.ts 100.00% <100.00%> (ø)
src/ast/nodes/Identifier.ts 100.00% <100.00%> (ø)
src/ast/nodes/ObjectPattern.ts 100.00% <ø> (ø)
src/ast/nodes/Property.ts 94.73% <ø> (ø)
src/ast/nodes/RestElement.ts 100.00% <ø> (ø)
src/ast/nodes/VariableDeclaration.ts 98.09% <ø> (ø)
... and 22 more

... and 1 file with indirect coverage changes

lukastaegert
lukastaegert reviewed Oct 22, 2023
View reviewed changes
@sapphi-red
Copy link
Contributor Author

sapphi-red commented Oct 23, 2023

Thanks for the review! I'm currently a bit busy, so I would take a look next month.
(Feel free to directly edit the PR, if you want to push this forward earlier.)

@lukastaegert
Copy link
Member

lukastaegert commented Oct 23, 2023

Feel free to directly edit the PR, if you want to push this forward earlier.

If you do not mind, that is the route I will be going 😉

@lukastaegert lukastaegert force-pushed the perf/run-lint-while-constructing-nodes branch from 1e936fd to fc3c4b5 Compare November 2, 2023 05:46
@sapphi-red
Copy link
Contributor Author

sapphi-red commented Nov 8, 2023 •
edited
Loading

Thanks for changes! I'm starting to have some time. Are there any places where it'd be better for me to work on?

@lukastaegert
Copy link
Member

lukastaegert commented Nov 8, 2023

I have some more I want to push tomorrow. Hopefully, I have it working by then. I added a ton of tests and even found some subtle bugs, and got a much better grasp on how catch scoping works...

@lukastaegert
Fix catch scoping
0c71403 
TODO: Parameter redeclaration
@lukastaegert
Copy link
Member

lukastaegert commented Nov 10, 2023

Ok, it is more complicated than I first thought. So if the declaration uses destructuring and/or is part of a comma-separated declaration for multiple variables, there is some work I need to put in here. I found a simple hack: I make it two separate variables but "link" them so that including one includes the other and they are always rendered with the same name.
In the future if we find time, we might look into implementing a logic that always splits declaration and initialization for hoisted variables, which would also allow for some new minor tree-shaking opportunities.

@lukastaegert
Copy link
Member

lukastaegert commented Nov 10, 2023

This is good to go from my side. If nothing else comes up, I would merge this tomorrow.

@sapphi-red
Copy link
Contributor Author

sapphi-red commented Nov 11, 2023

Thanks a lot! 💚
I pushed the refactors suggested in the review.
I also checked some test cases in acorn and tried with the playground. There were some errors in some cases (#5244, swc-project/swc#8268, swc-project/swc#8269). But these errors happened with 4.3.0 too, so I think this PR isn't related directly 👍 .

@lukastaegert
Copy link
Member

lukastaegert commented Nov 11, 2023

Great work, especially for checking the acorn test cases ❤️
So there is still some way to go with static checks, but I agree. I will have one last look if handling missing exports is a low-hanging fruit.

@lukastaegert
Improve handling of missing entry exports
d97611e 
Otherwise, it will throw an internal error
@lukastaegert
Copy link
Member

lukastaegert commented Nov 11, 2023

I changed the handling of missing entry exports so that it shows a proper error instead of this bug. This error will also be shown for invalid cases like export {default}, but it is definitely better than before. It will not flag this as an error in non-entry modules unless the user tries to import that specific export. But at least no invalid code will be generated as otherwise, the export will just be removed. Still, I think at some point in the future we might want to flag invalid exports in files as syntax errors.

@lukastaegert lukastaegert added this pull request to the merge queue Nov 12, 2023
Merged via the queue into rollup:master with commit fe6cb3a Nov 12, 2023
@github-actions
Copy link

github-actions bot commented Nov 12, 2023

This PR has been released as part of rollup@4.4.0. You can test it via npm install rollup.

@github-actions github-actions bot mentioned this pull request Nov 12, 2023
Closed
@sapphi-red sapphi-red deleted the perf/run-lint-while-constructing-nodes branch December 30, 2023 08:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lukastaegert lukastaegert lukastaegert approved these changes

Assignees

No one assigned

Labels

x⁸ ⚙️ build repl artefacts

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

"Cannot read properties of null (reading 'deoptimizePath')" error happens

2 participants

@sapphi-red @lukastaegert