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

refactor(traverse): Traverse produce scopes tree using Semantic #3304

Conversation

overlookmotel
Copy link
Collaborator

@overlookmotel overlookmotel commented May 16, 2024

Traverse use Semantic to construct scopes tree and expose it to visitors via TraverseCtx.

Currently scopes tree is immutable. Will expose it as a mutable in a follow-on.

This is extremely inefficient. Semantic does all kinds of stuff (control flow graph etc) which Traverse doesn't need, and Traverse just throws away all that work after semantic has done it. Intent here is to get a working implementation first, and then to do another pass later on to improve performance.

Copy link

graphite-app bot commented May 16, 2024

Your org has enabled the Graphite merge queue for merging into main

Add the label “merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

@github-actions github-actions bot added the A-transformer Area - Transformer / Transpiler label May 16, 2024
Copy link

codspeed-hq bot commented May 16, 2024

CodSpeed Performance Report

Merging #3304 will degrade performances by 66.05%

Comparing 05-16-refactor_traverse_traverse_produce_scopes_tree_using_semantic_ (05c71d2) with main (c8f1f79)

Summary

❌ 5 regressions
✅ 22 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main 05-16-refactor_traverse_traverse_produce_scopes_tree_using_semantic_ Change
transformer[RadixUIAdoptionSection.jsx] 530 µs 970.8 µs -45.41%
transformer[antd.js] 653.3 ms 1,605.8 ms -59.32%
transformer[cal.com.tsx] 159.2 ms 391.4 ms -59.32%
transformer[checker.ts] 341.7 ms 1,006.6 ms -66.05%
transformer[pdf.mjs] 105.1 ms 282.9 ms -62.86%

@overlookmotel overlookmotel force-pushed the 05-16-refactor_traverse_traverse_produce_scopes_tree_using_semantic_ branch 3 times, most recently from e44b95f to 3c02f60 Compare May 16, 2024 10:48
@overlookmotel overlookmotel changed the base branch from 05-16-refactor_semantic_semantic_populate_scope_id_fields_in_ast to main May 16, 2024 11:00
@overlookmotel overlookmotel force-pushed the 05-16-refactor_traverse_traverse_produce_scopes_tree_using_semantic_ branch from 3c02f60 to 25b4d44 Compare May 16, 2024 11:18
@overlookmotel overlookmotel changed the base branch from main to 05-16-refactor_semantic_semantic_populate_scope_id_fields_in_ast May 16, 2024 11:18
@overlookmotel overlookmotel force-pushed the 05-16-refactor_traverse_traverse_produce_scopes_tree_using_semantic_ branch from 25b4d44 to bdbe6fa Compare May 16, 2024 13:08
Copy link

graphite-app bot commented May 16, 2024

Merge activity

…3304)

`Traverse` use `Semantic` to construct scopes tree and expose it to visitors via `TraverseCtx`.

Currently scopes tree is immutable. Will expose it as a mutable in a follow-on.

This is extremely inefficient. Semantic does all kinds of stuff (control flow graph etc) which `Traverse` doesn't need, and `Traverse` just throws away all that work after semantic has done it. Intent here is to get a working implementation first, and then to do another pass later on to improve performance.
@Boshen Boshen force-pushed the 05-16-refactor_semantic_semantic_populate_scope_id_fields_in_ast branch from da7bf94 to 6f3b1c8 Compare May 16, 2024 16:22
@Boshen Boshen force-pushed the 05-16-refactor_traverse_traverse_produce_scopes_tree_using_semantic_ branch from bdbe6fa to 05c71d2 Compare May 16, 2024 16:22
Boshen pushed a commit that referenced this pull request May 16, 2024
Allow mutable access to scopes tree and symbol table.

Closes #3189.

This completes the v1 scopes-in-traverse implementation, and provides all the primitives required to implement the missing APIs listed in #3251.

Performance is abysmal, as noted in #3304, but we can fix that later on by taking `Semantic` out of the picture, or optimizing it.
@Boshen Boshen changed the base branch from 05-16-refactor_semantic_semantic_populate_scope_id_fields_in_ast to main May 16, 2024 16:29
@graphite-app graphite-app bot merged commit 05c71d2 into main May 16, 2024
26 checks passed
@graphite-app graphite-app bot deleted the 05-16-refactor_traverse_traverse_produce_scopes_tree_using_semantic_ branch May 16, 2024 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-transformer Area - Transformer / Transpiler
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants