refactor(ast)!: store span details inside comment struct #4132

lucab · 2024-07-09T06:14:49Z

This tweaks Comment definition in order to internally store the start and end position of its span.

Closes: #4069

graphite-app · 2024-07-09T06:14:54Z

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.

codspeed-hq · 2024-07-09T06:26:09Z

CodSpeed Performance Report

Merging #4132 will degrade performances by 3.22%

_{Comparing lucab:ups/comment-span (019489a) with main (714bf1d)}

Summary

❌ 2 regressions
✅ 27 untouched benchmarks

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

Benchmarks breakdown

	Benchmark	`main`	`lucab:ups/comment-span`	Change
❌	`semantic[antd.js]`	12.4 s	12.8 s	-3.22%
❌	`semantic[checker.ts]`	2.6 s	2.7 s	-3.21%

crates/oxc_ast/src/trivia.rs

Boshen · 2024-07-09T07:46:42Z

Weird, where's the performance regression coming from ...

crates/oxc_ast/src/trivia.rs

overlookmotel · 2024-07-09T08:53:15Z

Is comments_range on a hot path? If so, we could experiment with storing comments in SOA style with separate Vecs for start, end and kind. This might speed up searching (or maybe not, I can't really imagine what the memory access patterns are).

Or maybe just move CommentKind into a separate Vec<CommentKind>? Then it'd consume 9 bytes per comment, instead of 12, and searching wouldn't be looking at CommentKind which isn't relevant.

I still think the Comment type which is returned to caller should be as it is after this PR (as it's the most user-friendly format), but the backing storage could be different.

Boshen · 2024-07-09T10:02:34Z

Is comments_range on a hot path?

It seems to be on the wrong hot path, I need to fix it. The problem is unrelated to this PR.

This tweaks `Comment` definition in order to internally store the start and end position of its span.

graphite-app · 2024-07-09T15:20:57Z

Merge activity

Jul 9, 11:20 AM EDT: The merge label 'merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
Jul 9, 11:21 AM EDT: Boshen added this pull request to the Graphite merge queue.
Jul 9, 11:21 AM EDT: The Graphite merge queue couldn't merge this PR because it failed for an unknown reason (Stack merges are not currently supported for forked repositories. Please create a branch in the target repository in order to merge).
Jul 9, 11:33 AM EDT: The merge label 'merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
Jul 9, 3:38 PM UTC: The merge label 'merge' was removed. This PR will no longer be merged by the Graphite merge queue

Boshen · 2024-07-09T15:22:14Z

Comments processing shouldn't be on a hot path, merging this one and fixing the hot path problem in #4140

Minor followup from #4132.

## [0.20.0] - 2024-07-11 - 5731e39 ast: [**BREAKING**] Store span details inside comment struct (#4132) (Luca Bruno) ### Features - 67fe75e ast, ast_codegen: Pass the `scope_id` to the `enter_scope` event. (#4168) (rzvxa) - 54cd04a minifier: Implement dce with var hoisting (#4160) (Boshen) - 44a894a minifier: Implement return statement dce (#4155) (Boshen) - 725571a napi/transformer: Add `jsx` option to force parsing with jsx (#4133) (Boshen) ### Bug Fixes - 48947a2 ast: Put `decorators` before everything else. (#4143) (rzvxa) - 7a059ab cfg: Double resolution of labeled statements. (#4177) (rzvxa) - 4a656c3 lexer: Incorrect lexing of large hex/octal/binary literals (#4072) (DonIsaac) - 28eeee0 parser: Fix asi error diagnostic pointing at invalid text causing crash (#4163) (Boshen) ### Performance - ddfa343 diagnostic: Use `Cow<'static, str>` over `String` (#4175) (DonIsaac) - 2203143 semantic: Store unresolved refs in a stack (#4162) (lucab) - fca9706 semantic: Faster search for leading comments (#4140) (Boshen) ### Documentation - bdcc298 ast: Update the note regarding the `ast_codegen` markers. (#4149) (rzvxa) ### Refactor - 03ad1e3 semantic: Tweak comment argument type (#4157) (lucab) Co-authored-by: Boshen <Boshen@users.noreply.github.com>

## [0.6.0] - 2024-07-11 - 5731e39 ast: [**BREAKING**] Store span details inside comment struct (#4132) (Luca Bruno) ### Features - fb549e1 linter: Add vitest/no-focused-tests rule (#4178) (mysteryven) - 6c49007 linter: Add fixer for @typescript-eslint/consistent-type-imports (#3984) (mysteryven) - 278c3e9 linter: Add fixer for jsx-a11y/aria-props (#4176) (DonIsaac) - 2188144 linter: Eslint-plugin-jest/prefer-hooks-in-order (#4052) (cinchen) - cc58614 linter: Better schemas for allow/warn/deny (#4150) (DonIsaac) - c5b4be0 linter: Add fixer for prefer-node-protocol (#4129) (DonIsaac) - 7ec0c0b linter/eslint: Implement no-label-var (#4087) (Jelle van der Waa) ### Bug Fixes - ed4c54c eslint/radix: Detect yield Number.parseInt variant (#4110) (Jelle van der Waa) - e9ad03b linter: Fixer for no-debugger creates incorrect code (#4184) (DonIsaac) - bd69571 linter: Fix top level return panic in eslint/array_callback_return (#4167) (Boshen) - c8f5664 linter: Fix panic with unicode in unicorn/prefer_dom_node_dataset (#4166) (Boshen) - f2b3273 linter: Fix fixer panic in typescript/consistent_indexed_object_style (#4165) (Boshen) - 2334515 linter: Panic in `get_enclosing_function` (#4121) (DonIsaac) - 1b91d40 linter: Incorrect fixer for `no-unused-labels` (#4123) (Don Isaac) - 1729249 linter: Incorrect fix in `no-single-promise-in-promise-methods` rule; (#4094) (DonIsaac) - cc7e893 linter/tree-shaking: Avoid recursive function stackoverflow (#4191) (mysteryven) - 28eeee0 parser: Fix asi error diagnostic pointing at invalid text causing crash (#4163) (Boshen) - 0f02608 semantic: Bind `TSImportEqualsDeclaration`s (#4100) (Don Isaac) ### Performance - ddfa343 diagnostic: Use `Cow<'static, str>` over `String` (#4175) (DonIsaac) ### Refactor - 2687ebc react: Use find_binding helper for finding React binding (#4108) (Jelle van der Waa) Co-authored-by: Boshen <Boshen@users.noreply.github.com>

github-actions bot added A-linter Area - Linter A-parser Area - Parser A-semantic Area - Semantic A-ast Area - AST A-codegen Area - Code Generation labels Jul 9, 2024

Boshen reviewed Jul 9, 2024

View reviewed changes

crates/oxc_ast/src/trivia.rs Outdated Show resolved Hide resolved

overlookmotel reviewed Jul 9, 2024

View reviewed changes

crates/oxc_ast/src/trivia.rs Outdated Show resolved Hide resolved

overlookmotel reviewed Jul 9, 2024

View reviewed changes

crates/oxc_ast/src/trivia.rs Outdated Show resolved Hide resolved

overlookmotel reviewed Jul 9, 2024

View reviewed changes

crates/oxc_ast/src/trivia.rs Show resolved Hide resolved

lucab added 4 commits July 9, 2024 16:10

refactor(ast)!: store span details inside comment struct

0f5e3e0

This tweaks `Comment` definition in order to internally store the start and end position of its span.

ast: inline span getter

9b2d773

ast: change comment iterator item type

e8f7dbe

comment: make span public

0c7e4fc

lucab force-pushed the ups/comment-span branch from 34db466 to 0c7e4fc Compare July 9, 2024 14:12

github-actions bot added A-transformer Area - Transformer / Transpiler A-prettier Area - Prettier labels Jul 9, 2024

lucab added 2 commits July 9, 2024 16:17

wasm: fix iterator type

28c79b9

prettier: fix clippy lint

019489a

Boshen added the 0-merge Merge with Graphite Merge Queue label Jul 9, 2024

graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Jul 9, 2024

Boshen merged commit 5731e39 into oxc-project:main Jul 9, 2024
27 checks passed

Boshen added the 0-merge Merge with Graphite Merge Queue label Jul 9, 2024 — with Graphite App

lucab deleted the ups/comment-span branch July 9, 2024 15:34

lucab mentioned this pull request Jul 10, 2024

refactor(semantic): tweak comment argument type #4157

Merged

Boshen pushed a commit that referenced this pull request Jul 10, 2024

refactor(semantic): tweak comment argument type (#4157)

03ad1e3

Minor followup from #4132.

github-actions bot mentioned this pull request Jul 11, 2024

Release crates v0.20.0 #4189

Merged

github-actions bot mentioned this pull request Jul 11, 2024

Release oxlint v0.6.0 #4194

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(ast)!: store span details inside comment struct #4132

refactor(ast)!: store span details inside comment struct #4132

lucab commented Jul 9, 2024

graphite-app bot commented Jul 9, 2024

codspeed-hq bot commented Jul 9, 2024 •

edited

Loading

Boshen commented Jul 9, 2024

overlookmotel commented Jul 9, 2024 •

edited

Loading

Boshen commented Jul 9, 2024

graphite-app bot commented Jul 9, 2024 •

edited

Loading

Boshen commented Jul 9, 2024

refactor(ast)!: store span details inside comment struct #4132

refactor(ast)!: store span details inside comment struct #4132

Conversation

lucab commented Jul 9, 2024

graphite-app bot commented Jul 9, 2024

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

codspeed-hq bot commented Jul 9, 2024 • edited Loading

CodSpeed Performance Report

Merging #4132 will degrade performances by 3.22%

Summary

Benchmarks breakdown

Boshen commented Jul 9, 2024

overlookmotel commented Jul 9, 2024 • edited Loading

Boshen commented Jul 9, 2024

graphite-app bot commented Jul 9, 2024 • edited Loading

Merge activity

Boshen commented Jul 9, 2024

codspeed-hq bot commented Jul 9, 2024 •

edited

Loading

overlookmotel commented Jul 9, 2024 •

edited

Loading

graphite-app bot commented Jul 9, 2024 •

edited

Loading