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.

Sign up for GitHub

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

Jump to bottom

Collect comments around parentheses in expressions #13803

Merged
merged 5 commits into from Oct 5, 2021
Merged

Collect comments around parentheses in expressions #13803

merged 5 commits into from Oct 5, 2021

Conversation

nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Sep 30, 2021 •
edited by gitpod-io bot

Q                       A
Fixed Issues? Fixes #13750 (@bcoe); fixes a regression in AMP reported on slack (@jridgewell)
Patch: Bug Fix? Yes
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

This patch allows considering comments in /* foo */ (expr) as leading comments of expr, rather than as inner comments of the outer expression/statement.

This is needed for two reasons:

  • Tools using comments to annotate things have been broken by v7.14.8, who stopped considering them as leading comments
  • We don't have good support for printing inner comments. Marking them as leading fixes most of the problems, even if they get printed after (.

@JLHwung do you think this approach is viable?

@nicolo-ribaudo nicolo-ribaudo added PR: Bug Fix 🐛 A type of pull request used for our changelog categories i: discussion pkg: parser area: comments labels Sep 30, 2021
@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 30, 2021 •
edited

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 418cc3a:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration

@babel-bot
Copy link
Collaborator

babel-bot commented Sep 30, 2021 •
edited

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/49004/

JLHwung
JLHwung approved these changes Oct 4, 2021
View reviewed changes
Copy link
Contributor

@JLHwung JLHwung left a comment

Choose a reason for hiding this comment

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

LGTM except for the performance regression.

packages/babel-parser/src/parser/comments.js Outdated
node: Node,
comments: Array<Comment>,
) {
if (node[`${position}Comments`] === undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I can see there is 10% performance regression on the case babel-parser/many-leading-trailing-comments.

baseline 128 leading comments + 127 trailing comments: 8_315 ops/sec ±70.19% (0.12ms)
baseline 256 leading comments + 255 trailing comments: 6_601 ops/sec ±1.35% (0.151ms)
baseline 512 leading comments + 511 trailing comments: 3_211 ops/sec ±1.74% (0.311ms)
baseline 1024 leading comments + 1023 trailing comments: 1_530 ops/sec ±2.13% (0.653ms)
current 128 leading comments + 127 trailing comments: 8_055 ops/sec ±62.78% (0.124ms)
current 256 leading comments + 255 trailing comments: 5_767 ops/sec ±1.95% (0.173ms)
current 512 leading comments + 511 trailing comments: 2_837 ops/sec ±1.85% (0.353ms)
current 1024 leading comments + 1023 trailing comments: 1_378 ops/sec ±2.37% (0.726ms)

The generated machine code of setComments is inlined in serveral functions, such as finishNodeAt and finishNode. Here is what it looks like

0x10d0be84d   36d  48bee190bafba8090000 REX.W movq rsi,0x9a8fbba90e1    ;; object: 0x09a8fbba90e1 <FunctionContext[192]>
0x10d0be857   377  488bd1         REX.W movq rdx,rcx
0x10d0be85a   37a  48b9b16a6b7fa8090000 REX.W movq rcx,0x9a87f6b6ab1    ;; object: 0x09a87f6b6ab1 <String[16]: c"trailingComments">
0x10d0be864   384  b802000000     movl rax,0x2
0x10d0be869   389  48bb211c4261a8090000 REX.W movq rbx,0x9a861421c21    ;; object: 0x09a861421c21 <FeedbackVector[13]>
0x10d0be873   393  4c8bc3         REX.W movq r8,rbx
0x10d0be876   396  49ba20601d0301000000 REX.W movq r10,0x1031d6020  (KeyedLoadIC_Megamorphic)
0x10d0be880   3a0  41ffd2         call r10
0x10d0be883   3a3  49394510       REX.W cmpq [r13+0x10] (root (undefined_value)),rax
0x10d0be887   3a7  0f859d010000   jnz 0x10d0bea2a  <+0x54a>
0x10d0be88d   3ad  48b9916a6b7fa8090000 REX.W movq rcx,0x9a87f6b6a91    ;; object: 0x09a87f6b6a91 <String[16]: c"trailingComments">
0x10d0be897   3b7  bf08000000     movl rdi,0x8
0x10d0be89c   3bc  488b55a0       REX.W movq rdx,[rbp-0x60]
0x10d0be8a0   3c0  488b45a8       REX.W movq rax,[rbp-0x58]
0x10d0be8a4   3c4  48bb211c4261a8090000 REX.W movq rbx,0x9a861421c21    ;; object: 0x09a861421c21 <FeedbackVector[13]>
0x10d0be8ae   3ce  48bee190bafba8090000 REX.W movq rsi,0x9a8fbba90e1    ;; object: 0x09a8fbba90e1 <FunctionContext[192]>
0x10d0be8b8   3d8  49ba20bd1d0301000000 REX.W movq r10,0x1031dbd20  (KeyedStoreIC)
0x10d0be8c2   3e2  41ffd2         call r10

As we can see, V8 did computes ${position}Comments based on the input feedback. However, the fast property access is degraded to slow access (KeyedLoadIC_Megamorphic), which should be responsible for the performance regression here since setComments is a hot path in the benchmark case.

We can fix the regression here by copying setLeadingComments from setInnerComments and setTrailingComments:

baseline 128 leading comments + 127 trailing comments: 9_021 ops/sec ±64.25% (0.111ms)
baseline 256 leading comments + 255 trailing comments: 6_875 ops/sec ±0.83% (0.145ms)
baseline 512 leading comments + 511 trailing comments: 3_405 ops/sec ±0.68% (0.294ms)
baseline 1024 leading comments + 1023 trailing comments: 1_647 ops/sec ±1.29% (0.607ms)
current 128 leading comments + 127 trailing comments: 9_723 ops/sec ±62.49% (0.103ms)
current 256 leading comments + 255 trailing comments: 7_085 ops/sec ±0.7% (0.141ms)
current 512 leading comments + 511 trailing comments: 3_488 ops/sec ±0.58% (0.287ms)
current 1024 leading comments + 1023 trailing comments: 1_631 ops/sec ±1.57% (0.613ms)

nicolo-ribaudo
nicolo-ribaudo commented Oct 4, 2021
View reviewed changes
packages/babel-parser/src/parser/comments.js
if (node.innerComments === undefined) {
node.innerComments = comments;
} else if (comments !== undefined) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is just to align the signature to the other two functions: since they are really similar it's easier to keep the implementation the same.

@nicolo-ribaudo @existentialism
Update packages/babel-parser/src/parser/comments.js
418cc3a 
Co-authored-by: Brian Ng <bng412@gmail.com>
@nicolo-ribaudo nicolo-ribaudo changed the title Allow putting leading comments before parentheses in expressions Collect comments around parentheses in expressions Oct 5, 2021
@nicolo-ribaudo nicolo-ribaudo merged commit 64f14b0 into babel:main Oct 5, 2021
@nicolo-ribaudo nicolo-ribaudo deleted the leading-comments-parenthesized branch October 5, 2021 18:15
@snyk-bot snyk-bot mentioned this pull request Dec 14, 2021
Open
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Jan 5, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jridgewell jridgewell jridgewell left review comments

@existentialism existentialism existentialism approved these changes

@JLHwung JLHwung JLHwung approved these changes

Assignees
No one assigned
Labels
area: comments outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: parser PR: Bug Fix 🐛 A type of pull request used for our changelog categories
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: @babel/parser@7.14.8 breaks IstanbulJS skip logic
5 participants
@nicolo-ribaudo @babel-bot @existentialism @jridgewell @JLHwung