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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ae97220
Allow putting leading comments before parentheses in expressions
nicolo-ribaudo Sep 30, 2021
62ff104
Review
nicolo-ribaudo Oct 2, 2021
0d1c954
Update tests
nicolo-ribaudo Oct 2, 2021
7ba50d0
Fix perf
nicolo-ribaudo Oct 4, 2021
418cc3a
Update packages/babel-parser/src/parser/comments.js
nicolo-ribaudo Oct 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 11 additions & 0 deletions .../regression/comment-before-parentheses-return-arg-createParenthesizedExpressions/input.js
View file
Open in desktop
@@ -0,0 +1,11 @@
function assertElement(assertFn, shouldBeElement, opt_message) {
return /** @type {!Ele ment} */ (
nicolo-ribaudo marked this conversation as resolved.
Show resolved Hide resolved
assertType_(
assertFn,
shouldBeElement,
isElement(shouldBeElement),
'Element expected',
opt_message
)
);
}
4 changes: 4 additions & 0 deletions ...ression/comment-before-parentheses-return-arg-createParenthesizedExpressions/options.json
View file
Open in desktop
@@ -0,0 +1,4 @@
{
"retainLines": true,
"createParenthesizedExpressions": true
}
11 changes: 11 additions & 0 deletions ...regression/comment-before-parentheses-return-arg-createParenthesizedExpressions/output.js
View file
Open in desktop
@@ -0,0 +1,11 @@
function assertElement(assertFn, shouldBeElement, opt_message) {
return (/** @type {!Ele ment} */
assertType_(
assertFn,
shouldBeElement,
isElement(shouldBeElement),
'Element expected',
opt_message));


}
11 changes: 11 additions & 0 deletions ...s/babel-generator/test/fixtures/regression/comment-before-parentheses-return-arg/input.js
View file
Open in desktop
@@ -0,0 +1,11 @@
function assertElement(assertFn, shouldBeElement, opt_message) {
nicolo-ribaudo marked this conversation as resolved.
Show resolved Hide resolved
return /** @type {!Ele ment} */ (
assertType_(
assertFn,
shouldBeElement,
isElement(shouldBeElement),
'Element expected',
opt_message
)
);
}
6 changes: 6 additions & 0 deletions .../babel-generator/test/fixtures/regression/comment-before-parentheses-return-arg/output.js
View file
Open in desktop
@@ -0,0 +1,6 @@
function assertElement(assertFn, shouldBeElement, opt_message) {
return (
/** @type {!Ele ment} */
assertType_(assertFn, shouldBeElement, isElement(shouldBeElement), 'Element expected', opt_message)
);
}
57 changes: 46 additions & 11 deletions packages/babel-parser/src/parser/comments.js
View file
Open in desktop
Expand Up @@ -26,22 +26,31 @@ export type CommentWhitespace = {
trailingNode: Node | null,
containingNode: Node | null,
};

/**
* Merge comments with node's trailingComments or assign comments to be
* trailingComments. New comments will be placed before old comments
* Merge comments with node's ${type}Comments or assign comments to be
* ${type}Comments. New comments will be placed before old comments
* because the commentStack is enumerated reversely.
*
* @param {"leading" | "inner" | "trailing"} position
* @param {Node} node
* @param {Array<Comment>} comments
*/
function setTrailingComments(node: Node, comments: Array<Comment>) {
if (node.trailingComments === undefined) {
node.trailingComments = comments;
function setComments(
position: "leading" | "inner" | "trailing",
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)

node[`${position}Comments`] = comments;
} else {
node.trailingComments.unshift(...comments);
node[`${position}Comments`].unshift(...comments);
}
}

const setLeadingComments = setComments.bind(null, "leading");
nicolo-ribaudo marked this conversation as resolved.
Show resolved Hide resolved
const setTrailingComments = setComments.bind(null, "trailing");

/**
* Merge comments with node's innerComments or assign comments to be
* innerComments. New comments will be placed before old comments
Expand All @@ -51,10 +60,8 @@ function setTrailingComments(node: Node, comments: Array<Comment>) {
* @param {Array<Comment>} comments
*/
export function setInnerComments(node: Node, comments: Array<Comment> | void) {
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.

node.innerComments.unshift(...comments);
if (comments) {
setComments("inner", node, comments);
}
}

Expand Down Expand Up @@ -149,7 +156,7 @@ export default class CommentsParser extends BaseParser {
setTrailingComments(commentWS.leadingNode, comments);
}
if (commentWS.trailingNode !== null) {
commentWS.trailingNode.leadingComments = comments;
setLeadingComments(commentWS.trailingNode, comments);
}
} else {
/*:: invariant(commentWS.containingNode !== null) */
Expand Down Expand Up @@ -238,4 +245,32 @@ export default class CommentsParser extends BaseParser {
commentWS.leadingNode = null;
}
}

/**
* Marks the node as a trailingNode of the comment whitespace
* ending at pos, if it exists.
*
* This is used to propertly attach comments before parenthesized
* expressions as leading comments of the inner expression.
*
* @param {Node} node
* @param {number} pos
*/
takeLeadingCommentsBefore(node: Node, pos: number) {
const { commentStack } = this.state;
const commentStackLength = commentStack.length;
if (commentStackLength === 0) return;
let i = commentStackLength - 1;

for (; i >= 0; i--) {
const commentWS = commentStack[i];
const commentEnd = commentWS.end;

if (commentEnd === pos) {
commentWS.trailingNode = node;
} else if (commentEnd < pos) {
break;
}
}
}
}
3 changes: 3 additions & 0 deletions packages/babel-parser/src/parser/expression.js
View file
Open in desktop
Expand Up @@ -1738,6 +1738,9 @@ export default class ExpressionParser extends LValParser {
if (!this.options.createParenthesizedExpressions) {
this.addExtra(val, "parenthesized", true);
this.addExtra(val, "parenStart", startPos);

this.takeLeadingCommentsBefore(val, startPos);
nicolo-ribaudo marked this conversation as resolved.
Show resolved Hide resolved

return val;
}

Expand Down
10 changes: 5 additions & 5 deletions packages/babel-parser/test/fixtures/comments/basic/sequence-expression/output.json
View file
Open in desktop
Expand Up @@ -25,11 +25,6 @@
}
],
"innerComments": [
{
"type": "CommentBlock",
"value": " 2 ",
"start":9,"end":16,"loc":{"start":{"line":1,"column":9},"end":{"line":1,"column":16}}
},
{
"type": "CommentBlock",
"value": " 8 ",
Expand All @@ -40,6 +35,11 @@
"type": "SequenceExpression",
"start":27,"end":47,"loc":{"start":{"line":1,"column":27},"end":{"line":1,"column":47}},
"leadingComments": [
{
"type": "CommentBlock",
"value": " 2 ",
"start":9,"end":16,"loc":{"start":{"line":1,"column":9},"end":{"line":1,"column":16}}
},
{
"type": "CommentBlock",
"value": " 3 ",
Expand Down
5 changes: 5 additions & 0 deletions packages/babel-parser/test/fixtures/comments/regression/13750/input.js
View file
Open in desktop
@@ -0,0 +1,5 @@
if (args[0] === 1 || /* istanbul ignore next */ (args[0] === 2 || args[0] === 3)) {
nicolo-ribaudo marked this conversation as resolved.
Show resolved Hide resolved
output = args[0] + 10;
} else {
output = 20;
}