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

Replace generic __clone call by specific methods #13611

Merged
merged 4 commits into from Jul 30, 2021
Merged

Replace generic __clone call by specific methods #13611

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
de0fe04
update benchmark babel parser version
JLHwung Jul 28, 2021
2d44607
perf: replace generic __clone by specific methods
JLHwung Jul 28, 2021
fc9a8af
breaking: remove Node#__clone in Babel 8
JLHwung Jul 28, 2021
4bbf05d
test: use t.cloneNode
JLHwung Jul 28, 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
2 changes: 1 addition & 1 deletion benchmark/package.json
View file
Open in desktop
Expand Up @@ -5,7 +5,7 @@
"devDependencies": {
"@babel-baseline/generator": "npm:@babel/generator@7.14.5",
"@babel-baseline/helper-validator-identifier": "npm:@babel/helper-validator-identifier@7.10.4",
"@babel-baseline/parser": "npm:@babel/parser@7.14.5",
"@babel-baseline/parser": "npm:@babel/parser@7.14.8",
"@babel/generator": "workspace:*",
"@babel/helper-validator-identifier": "workspace:*",
"@babel/parser": "workspace:*",
Expand Down
7 changes: 4 additions & 3 deletions packages/babel-parser/src/parser/expression.js
View file
Open in desktop
Expand Up @@ -57,6 +57,7 @@ import {
import { Errors, SourceTypeModuleErrors } from "./error";
import type { ParsingError } from "./error";
import { setInnerComments } from "./comments";
import { cloneIdentifier } from "./node";

/*::
import type { SourceType } from "../options";
Expand Down Expand Up @@ -1938,7 +1939,7 @@ export default class ExpressionParser extends LValParser {
prop.value = this.parseMaybeDefault(
startPos,
startLoc,
prop.key.__clone(),
cloneIdentifier(prop.key),
);
} else if (this.match(tt.eq) && refExpressionErrors) {
if (refExpressionErrors.shorthandAssign === -1) {
Expand All @@ -1947,10 +1948,10 @@ export default class ExpressionParser extends LValParser {
prop.value = this.parseMaybeDefault(
startPos,
startLoc,
prop.key.__clone(),
cloneIdentifier(prop.key),
);
} else {
prop.value = prop.key.__clone();
prop.value = cloneIdentifier(prop.key);
}
prop.shorthand = true;

Expand Down
46 changes: 44 additions & 2 deletions packages/babel-parser/src/parser/node.js
View file
Open in desktop
Expand Up @@ -26,8 +26,12 @@ class Node implements NodeBase {
trailingComments: Array<Comment>;
innerComments: Array<Comment>;
extra: { [key: string]: any };
}
const NodePrototype = Node.prototype;

__clone(): this {
if (!process.env.BABEL_8_BREAKING) {
// $FlowIgnore
NodePrototype.__clone = function (): Node {
// $FlowIgnore
const newNode: any = new Node();
const keys = Object.keys(this);
Expand All @@ -39,13 +43,51 @@ class Node implements NodeBase {
key !== "trailingComments" &&
key !== "innerComments"
) {
// $FlowIgnore
newNode[key] = this[key];
}
}

return newNode;
};
}

function clonePlaceholder(node: any): any {
return cloneIdentifier(node);
}

export function cloneIdentifier(node: any): any {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately I can not get flow types working on this method. The Placeholder type complicates the typings here.

Copy link
Member

Choose a reason for hiding this comment

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

Does using generics work? Something like

export function cloneIdentifier<T: Identifier | Placeholder<"Identifier">>(node: T): T

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried generics. However it seems that Flow cannot infer the type from type === "Placeholder". So I fallback to invariant comments, however Flow will throw "unexpected )" at the invariant comment /*:: invariant(node instanceof Placeholder<"Identifier">) */.

Copy link
Member

Choose a reason for hiding this comment

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

Has the parser conversion to TS already started?

Copy link
Member

Choose a reason for hiding this comment

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

Not yet; you can see the wip at #11578

// We don't need to clone `typeAnnotations` and `optional`: because
// cloneIdentifier is only used in object shorthand and named import/export.
// Neither of them allow type annotations after the identifier or optional identifier
const { type, start, end, loc, range, extra, name } = node;
const cloned = Object.create(NodePrototype);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can use plain object after we remove __clone method from the AST node.

Copy link

@KFlash KFlash Jul 28, 2021 •
edited

Choose a reason for hiding this comment

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

why do you need to clone? You will experience more than 80% perf boost if you avoid clone and instead rewrite lexer and parser. And also all the property access you are using kills perf.

cloned.type = type;
cloned.start = start;
cloned.end = end;
cloned.loc = loc;
cloned.range = range;
cloned.extra = extra;
cloned.name = name;
if (type === "Placeholder") {
cloned.expectedNode = node.expectedNode;
}
return cloned;
}

export function cloneStringLiteral(node: any): any {
const { type, start, end, loc, range, extra } = node;
if (type === "Placeholder") {
return clonePlaceholder(node);
}
const cloned = Object.create(NodePrototype);
Copy link

Choose a reason for hiding this comment

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

'Object.create' is so much slower than a constructor. And overall it's slow.

https://stackoverflow.com/questions/43372746/why-so-bad-the-performance-of-the-nodejs-object-creation-if-num-of-props-greater

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We will remove Object.create here in Babel 8.

Copy link

@KFlash KFlash Jul 29, 2021 •
edited

Choose a reason for hiding this comment

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

Can't you just use the 'proto' trick?

const table = {
  __proto__: null,
  JLHwung: author
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

__proto__ is slower than Object.create:

baseline 256 length-1 named export: 5_650 ops/sec ±45.38% (0.177ms)
baseline 512 length-1 named export: 4_129 ops/sec ±1.16% (0.242ms)
baseline 1024 length-1 named export: 1_977 ops/sec ±1.31% (0.506ms)
baseline 2048 length-1 named export: 894 ops/sec ±2.1% (1.118ms)
current 256 length-1 named export: 5_269 ops/sec ±29.84% (0.19ms)
current 512 length-1 named export: 3_003 ops/sec ±2.24% (0.333ms)
current 1024 length-1 named export: 1_473 ops/sec ±2.04% (0.679ms)
current 2048 length-1 named export: 710 ops/sec ±1.6% (1.409ms)

Copy link

Choose a reason for hiding this comment

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

Interesting. I never clone so didn't check that. What I do is to have a create function that creates and return an obj based on the given params. When I need to "clone" I re-use that function and create a new obj based on the given params again. That way I avoid spread, clone and other slow stuff. And it makes GC happy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

create a new obj based on the given params again

I agree. But we should get rid of loc and ranges first so the parameters can be limited to start, end, value and type (solely for placeholder support).

Copy link

Choose a reason for hiding this comment

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

or just pack the loc data in a series of binary numbers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well there are also filename and identifierName in the loc.

cloned.type = "StringLiteral";
cloned.start = start;
cloned.end = end;
cloned.loc = loc;
cloned.range = range;
cloned.extra = extra;
cloned.value = node.value;
return cloned;
}

export class NodeUtils extends UtilParser {
Expand Down
17 changes: 12 additions & 5 deletions packages/babel-parser/src/parser/statement.js
View file
Open in desktop
Expand Up @@ -34,6 +34,7 @@ import {
import type { SourceType } from "../options";
import { Token } from "../tokenizer";
import { Position } from "../util/location";
import { cloneStringLiteral, cloneIdentifier } from "./node";

const loopLabel = { kind: "loop" },
switchLabel = { kind: "switch" };
Expand Down Expand Up @@ -2144,10 +2145,16 @@ export default class StatementParser extends ExpressionParser {
}

const node = this.startNode();
node.local = this.parseModuleExportName();
node.exported = this.eatContextual("as")
? this.parseModuleExportName()
: node.local.__clone();
const isString = this.match(tt.string);
nicolo-ribaudo marked this conversation as resolved.
Show resolved Hide resolved
const local = this.parseModuleExportName();
node.local = local;
if (this.eatContextual("as")) {
node.exported = this.parseModuleExportName();
} else if (isString) {
node.exported = cloneStringLiteral(local);
} else {
node.exported = cloneIdentifier(local);
}
nodes.push(this.finishNode(node, "ExportSpecifier"));
}

Expand Down Expand Up @@ -2423,7 +2430,7 @@ export default class StatementParser extends ExpressionParser {
);
}
this.checkReservedWord(imported.name, specifier.start, true, true);
specifier.local = imported.__clone();
specifier.local = cloneIdentifier(imported);
}
this.checkLVal(specifier.local, "import specifier", BIND_LEXICAL);
node.specifiers.push(this.finishNode(specifier, "ImportSpecifier"));
Expand Down
7 changes: 4 additions & 3 deletions packages/babel-parser/src/plugins/flow/index.js
View file
Open in desktop
Expand Up @@ -25,6 +25,7 @@ import {
} from "../../util/scopeflags";
import type { ExpressionErrors } from "../../parser/util";
import { Errors, makeErrorTemplates, ErrorCodes } from "../../parser/error";
import { cloneIdentifier } from "../../parser/node";

const reservedTypes = new Set([
"_",
Expand Down Expand Up @@ -2655,7 +2656,7 @@ export default (superClass: Class<Parser>): Class<Parser> =>
// `import {type as ,` or `import {type as }`
specifier.imported = as_ident;
specifier.importKind = specifierTypeKind;
specifier.local = as_ident.__clone();
specifier.local = cloneIdentifier(as_ident);
} else {
// `import {type as foo`
specifier.imported = firstIdent;
Expand All @@ -2673,7 +2674,7 @@ export default (superClass: Class<Parser>): Class<Parser> =>
specifier.local = this.parseIdentifier();
} else {
isBinding = true;
specifier.local = specifier.imported.__clone();
specifier.local = cloneIdentifier(specifier.imported);
}
} else {
if (firstIdentIsString) {
Expand All @@ -2688,7 +2689,7 @@ export default (superClass: Class<Parser>): Class<Parser> =>
isBinding = true;
specifier.imported = firstIdent;
specifier.importKind = null;
specifier.local = specifier.imported.__clone();
specifier.local = cloneIdentifier(specifier.imported);
}

const nodeIsTypeImport = hasTypeImportKind(node);
Expand Down
1 change: 1 addition & 0 deletions packages/babel-parser/src/types.js
View file
Open in desktop
Expand Up @@ -98,6 +98,7 @@ export type Identifier = PatternBase & {
type: "Identifier",
name: string,

// @deprecated
__clone(): Identifier,

// TypeScript only. Used in case of an optional parameter.
Expand Down
2 changes: 1 addition & 1 deletion packages/babel-traverse/test/replacement.js
View file
Open in desktop
Expand Up @@ -122,7 +122,7 @@ describe("path/replacement", function () {
OptionalMemberExpression(path) {
path.node.type = "MemberExpression";
// force `replaceWith` to replace `path.node`
path.replaceWith(path.node.__clone());
path.replaceWith(t.cloneNode(path.node));
path.parentPath.ensureBlock();

const aQuestionDotBNode = path.node.object.expression;
Expand Down
10 changes: 5 additions & 5 deletions yarn.lock
View file
Open in desktop
Expand Up @@ -23,12 +23,12 @@ __metadata:
languageName: node
linkType: hard

"@babel-baseline/parser@npm:@babel/parser@7.14.5":
version: 7.14.5
resolution: "@babel/parser@npm:7.14.5"
"@babel-baseline/parser@npm:@babel/parser@7.14.8":
version: 7.14.8
resolution: "@babel/parser@npm:7.14.8"
bin:
parser: ./bin/babel-parser.js
checksum: 55c14793888cb7d54275811e7f13136875df1ee4fc368f3f10cff46ebdf95b6a072e706a0486be0ac5686a597cbfb82f33b5f66aa6ba80ff50b73bca945035c6
checksum: 1f900e92675bac6120dfb3e9ea86841fdaba11d338a220017dbcb9e95815a3854ea479da8027e80acaa7f0e618b96e59bbbf3a230a05aaa3407c9419eb742cfe
languageName: node
linkType: hard

Expand Down Expand Up @@ -87,7 +87,7 @@ __metadata:
dependencies:
"@babel-baseline/generator": "npm:@babel/generator@7.14.5"
"@babel-baseline/helper-validator-identifier": "npm:@babel/helper-validator-identifier@7.10.4"
"@babel-baseline/parser": "npm:@babel/parser@7.14.5"
"@babel-baseline/parser": "npm:@babel/parser@7.14.8"
"@babel/generator": "workspace:*"
"@babel/helper-validator-identifier": "workspace:*"
"@babel/parser": "workspace:*"
Expand Down