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

feat(ast): use atom for Directive and Hashbang #701

Merged
merged 3 commits into from
Aug 9, 2023
Merged

Conversation

hyf0
Copy link
Contributor

@hyf0 hyf0 commented Aug 9, 2023

The main reason is using Atom to remove the lifetime for convenience.

And after removing the lifetime of these nodes, the Program<'a> doesn't rely on &'a source anymore, which allows us to specify more accurate lifetimes.

https://github.com/web-infra-dev/oxc/blob/f5b8690309c97f17e979dc232417006ffd75fbab/crates/oxc_parser/src/lib.rs#L135

https://github.com/web-infra-dev/oxc/blob/f5b8690309c97f17e979dc232417006ffd75fbab/crates/oxc_parser/src/lib.rs#L95

@hyf0 hyf0 changed the title feat(ast): use atom for Directive and Hashbang feat(ast): use atom for Directive and Hashbang Aug 9, 2023
@github-actions github-actions bot added A-parser Area - Parser A-minifier Area - Minifier A-printer Area - Printer labels Aug 9, 2023
crates/oxc_parser/src/js/statement.rs Outdated Show resolved Hide resolved
crates/oxc_parser/src/js/statement.rs Outdated Show resolved Hide resolved
@hyf0
Copy link
Contributor Author

hyf0 commented Aug 9, 2023

I'm not sure the use case. But more accurate lifetimes after this PR allow us to have the AST while being droped the source_text.

image

Copy link
Member

@Boshen Boshen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cargo fmt failed :-(

@Boshen
Copy link
Member

Boshen commented Aug 9, 2023

I'm not sure the use case. But more accurate lifetimes after this PR allow us to have the AST while being droped the source_text.

image

You can also allocator.alloc(source_text) I think.

@Boshen
Copy link
Member

Boshen commented Aug 9, 2023

I'm not sure the use case. But more accurate lifetimes after this PR allow us to have the AST while being droped the source_text.

image

I don't think it's related to this PR 🤔 Program still has lifetime 'a, do we need to fix the parser signature as well?

@codecov-commenter
Copy link

Codecov Report

Patch coverage: 95.00% and no project coverage change.

Comparison is base (f5b8690) 89.52% compared to head (98defb3) 89.52%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #701   +/-   ##
=======================================
  Coverage   89.52%   89.52%           
=======================================
  Files         205      205           
  Lines       42229    42229           
=======================================
  Hits        37804    37804           
  Misses       4425     4425           
Files Changed Coverage Δ
crates/oxc_ast/src/ast/js.rs 65.84% <ø> (ø)
crates/oxc_ast/src/ast_kind.rs 17.26% <0.00%> (ø)
crates/oxc_formatter/src/gen.rs 93.10% <ø> (ø)
crates/oxc_hir/src/hir.rs 34.26% <ø> (ø)
crates/oxc_hir/src/hir_kind.rs 0.00% <ø> (ø)
crates/oxc_minifier/src/printer/gen.rs 89.11% <ø> (ø)
crates/oxc_ast/src/ast_builder.rs 98.21% <100.00%> (ø)
crates/oxc_ast/src/visit.rs 99.20% <100.00%> (ø)
crates/oxc_ast_lower/src/lib.rs 99.36% <100.00%> (ø)
crates/oxc_hir/src/hir_builder.rs 87.43% <100.00%> (ø)
... and 3 more

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hyf0
Copy link
Contributor Author

hyf0 commented Aug 9, 2023

I don't think it's related to this PR 🤔 Program still has lifetime 'a, do we need to fix the parser signature as well?

No, it's not. It should be fixed in another PR(if you think this should be fixed), but that PR requires the changes in this PR.

do we need to fix the parser signature as well?

Yes. Probably something like Parser<'arena, 'source_txt>

@Boshen Boshen merged commit 3516759 into main Aug 9, 2023
18 checks passed
@Boshen Boshen deleted the hyf_32849230849023 branch August 9, 2023 05:52
@Boshen
Copy link
Member

Boshen commented Aug 9, 2023

Thank you!

@github-actions
Copy link
Contributor

github-actions bot commented Aug 9, 2023

Benchmark Results

Linux

group                            main                                   pr
-----                            ----                                   --
minifier/antd.js                 1.01    240.7±3.16ms    26.5 MB/sec    1.00    238.3±2.68ms    26.8 MB/sec
minifier/react.development.js    1.00      2.6±0.01ms    26.8 MB/sec    1.00      2.6±0.01ms    26.8 MB/sec
minifier/typescript.js           1.02    410.7±3.94ms    25.1 MB/sec    1.00    403.3±4.10ms    25.6 MB/sec
minifier/vue.js                  1.02     15.6±0.44ms    20.9 MB/sec    1.00     15.3±0.14ms    21.3 MB/sec
parser/antd.js                   1.01     76.7±1.18ms    83.1 MB/sec    1.00     76.1±1.09ms    83.8 MB/sec
parser/react.development.js      1.01   853.7±15.00µs    80.6 MB/sec    1.00    841.5±7.36µs    81.8 MB/sec
parser/typescript.js             1.01    131.6±0.40ms    78.4 MB/sec    1.00    130.5±0.41ms    79.1 MB/sec
parser/vue.js                    1.02      5.4±0.12ms    60.6 MB/sec    1.00      5.3±0.02ms    62.1 MB/sec
semantic/antd.js                 1.00     76.5±3.58ms    83.4 MB/sec    1.01     77.3±1.41ms    82.5 MB/sec
semantic/react.development.js    1.00   833.3±18.00µs    82.6 MB/sec    1.01   838.7±15.34µs    82.0 MB/sec
semantic/typescript.js           1.01    169.1±5.42ms    61.0 MB/sec    1.00    167.6±5.16ms    61.6 MB/sec
semantic/vue.js                  1.01      5.5±0.12ms    59.8 MB/sec    1.00      5.4±0.07ms    60.4 MB/sec

Windows

group                            main                                   pr
-----                            ----                                   --
minifier/antd.js                 1.06   320.1±11.89ms    19.9 MB/sec    1.00    301.0±9.50ms    21.2 MB/sec
minifier/react.development.js    1.08      3.6±1.84ms    19.1 MB/sec    1.00      3.3±0.03ms    20.6 MB/sec
minifier/typescript.js           1.02   487.2±14.35ms    21.2 MB/sec    1.00   479.3±10.75ms    21.5 MB/sec
minifier/vue.js                  1.00     20.3±0.46ms    16.1 MB/sec    1.00     20.3±0.32ms    16.1 MB/sec
parser/antd.js                   1.02     82.8±2.14ms    77.0 MB/sec    1.00     80.8±2.16ms    78.9 MB/sec
parser/react.development.js      1.02   892.5±12.26µs    77.1 MB/sec    1.00   878.0±10.11µs    78.4 MB/sec
parser/typescript.js             1.02    140.1±2.19ms    73.6 MB/sec    1.00    137.2±2.12ms    75.2 MB/sec
parser/vue.js                    1.01      5.8±0.04ms    56.5 MB/sec    1.00      5.7±0.07ms    57.0 MB/sec
semantic/antd.js                 1.06    87.5±10.75ms    72.9 MB/sec    1.00     82.8±3.72ms    77.0 MB/sec
semantic/react.development.js    1.00  1107.9±28.44µs    62.1 MB/sec    1.01  1123.5±33.28µs    61.2 MB/sec
semantic/typescript.js           1.02   204.0±16.02ms    50.6 MB/sec    1.00   199.0±11.30ms    51.8 MB/sec
semantic/vue.js                  1.00      7.2±0.11ms    45.0 MB/sec    1.01      7.3±0.11ms    44.8 MB/sec

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-minifier Area - Minifier A-parser Area - Parser A-printer Area - Printer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants