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

AST transfer WIP #2457

Draft
wants to merge 35 commits into
base: main
Choose a base branch
from
Draft

AST transfer WIP #2457

wants to merge 35 commits into from

Conversation

overlookmotel
Copy link
Contributor

@overlookmotel overlookmotel commented Feb 20, 2024

Very rough POC for #2409. Nowhere near ready to merge.

Mostly works. JS AST for checker.ts via "raw" transfer matches AST produced via current JSON API exactly. Some errors in AST for pdf.mjs and antd.js due to some of reasons listed below.

Edit 8/3/24: Output for pdf.mjs and antd.js is now correct.

Current deficiencies:

  • Incorrect output for files containing any non-ASCII characters.
  • Does not handle strings/identifiers/regexps containing \ escapes.
  • Statement and a few other types increase in size by 8 bytes (likely the cause of the performance regression seen in benchmarks).
  • Requires size of AST to be known in advance.
  • Hacky method of creating allocations.
  • Memory handling may be unsound!

@github-actions github-actions bot added the A-ast Area - AST label Feb 20, 2024
@overlookmotel
Copy link
Contributor Author

overlookmotel commented Feb 20, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @overlookmotel and the rest of your teammates on Graphite Graphite

Copy link

codspeed-hq bot commented Feb 20, 2024

CodSpeed Performance Report

Merging #2457 will improve performances by 4.52%

Comparing 02-20-AST_transfer_WIP (79d2808) with main (88ded0c)

Summary

⚡ 2 improvements
✅ 28 untouched benchmarks

🆕 5 new benchmarks

Benchmarks breakdown

Benchmark main 02-20-AST_transfer_WIP Change
🆕 parser_napi[RadixUIAdoptionSection.jsx] N/A 7.5 µs N/A
🆕 parser_napi[antd.js] N/A 24.3 ms N/A
🆕 parser_napi[cal.com.tsx] N/A 4.1 ms N/A
🆕 parser_napi[checker.ts] N/A 8.7 ms N/A
🆕 parser_napi[pdf.mjs] N/A 2.6 ms N/A
semantic[antd.js] 218.3 ms 208.8 ms +4.52%
semantic[pdf.mjs] 35.9 ms 34.6 ms +3.7%

@andymac4182
Copy link
Contributor

Does this mean we could use oxc_parser for eslint as well as prettier?

@overlookmotel
Copy link
Contributor Author

Maybe... Just to say this is very early stage work, the concept is not proven, and there is a long way to go yet. Some significant things would need to change within OXC to convert this from an experiment to a solid, dependable reality.

Personally, I do believe there is potential in this approach, but I think it's premature at this stage to talk a great deal about applications.

Sorry if that sounds negative. I'm just trying to give a realistic answer.

@andymac4182
Copy link
Contributor

That's okay 😀 that's a perfectly reasonably answer

@overlookmotel
Copy link
Contributor Author

overlookmotel commented Mar 15, 2024

I don't believe the Codspeed benchmarks are accurate. Running benchmarks locally, I'm seeing more like x3 - x5 speed-up. Will investigate.

@overlookmotel overlookmotel force-pushed the 02-20-AST_transfer_WIP branch 5 times, most recently from 8189817 to 7554739 Compare March 16, 2024 13:41
@overlookmotel
Copy link
Contributor Author

Yes, benchmarks on CodSpeed are completely wrong. Have raised an issue CodSpeedHQ/action#96

@overlookmotel overlookmotel force-pushed the 02-20-AST_transfer_WIP branch 4 times, most recently from f1c4d84 to ef6a6af Compare March 20, 2024 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ast Area - AST
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants