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

Don't cache child lists for tokens #58233

Merged
merged 6 commits into from
Apr 19, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions src/compiler/factory/nodeChildren.ts
Original file line number Diff line number Diff line change
@@ -1,14 +1,20 @@
import { Node } from "../_namespaces/ts";
import {
isToken,
Node,
} from "../_namespaces/ts";

const nodeChildren = new WeakMap<Node, Node[] | undefined>();
const nodeChildren = new WeakMap<Node, readonly Node[] | undefined>();

const emptyChildren: readonly Node[] = Object.freeze([]);
Copy link
Member

Choose a reason for hiding this comment

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

Why not use emptyArray?

Copy link
Member

Choose a reason for hiding this comment

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

Confusingly, services defines its own mutable empty array (and none of them actually guard against modification)

Copy link
Member

Choose a reason for hiding this comment

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

Oh, this isn't even in services, yeah, emptyArray would be nice. (I used to have a PR that froze it but it didn't really seem to matter)

Copy link
Member Author

Choose a reason for hiding this comment

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

From the description:

If we want, we can probably just use the internal emptyArray, but I don't think we've ever frozen that one.

Happy to switch over to that too, but Jake mentioned that he didn't want to make that change since getChildren() always was declared as a mutable array. Freezing this array (for runtime safety) and propagating the change (for static verification) helped validate that it's unlikely we mutate these arrays.

Copy link
Member

Choose a reason for hiding this comment

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

Does freezing emptyArray affect our perf at all? If not, I'd just do that and use emptyArray.

Copy link
Member

Choose a reason for hiding this comment

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

I tried in #55755 but I think that was actually more of a problem with trying to make immutable empty maps/sets. Sent #58240 to try just arrays quick.

Copy link
Member

Choose a reason for hiding this comment

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

it sure looks like freezing is bad: #58240 (comment)


/** @internal */
export function getNodeChildren(node: Node): Node[] | undefined {
export function getNodeChildren(node: Node): readonly Node[] | undefined {
if (isToken(node)) return emptyChildren;
return nodeChildren.get(node);
}

/** @internal */
export function setNodeChildren(node: Node, children: Node[]): Node[] {
export function setNodeChildren(node: Node, children: readonly Node[]): readonly Node[] {
nodeChildren.set(node, children);
return children;
}
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/factory/nodeFactory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6209,7 +6209,7 @@ export function createNodeFactory(flags: NodeFactoryFlags, baseFactory: BaseNode
}

// @api
function createSyntaxList(children: Node[]) {
function createSyntaxList(children: readonly Node[]) {
const node = createBaseNode<SyntaxList>(SyntaxKind.SyntaxList);
setNodeChildren(node, children);
return node;
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9006,7 +9006,7 @@ export interface NodeFactory {
// Synthetic Nodes
//
/** @internal */ createSyntheticExpression(type: Type, isSpread?: boolean, tupleNameSource?: ParameterDeclaration | NamedTupleMember): SyntheticExpression;
/** @internal */ createSyntaxList(children: Node[]): SyntaxList;
/** @internal */ createSyntaxList(children: readonly Node[]): SyntaxList;

//
// Transformation nodes
Expand Down
4 changes: 2 additions & 2 deletions src/services/services.ts
Original file line number Diff line number Diff line change
Expand Up @@ -438,7 +438,7 @@ class NodeObject<TKind extends SyntaxKind> implements Node {
return this.getChildren(sourceFile)[index];
}

public getChildren(sourceFile?: SourceFileLike): Node[] {
public getChildren(sourceFile?: SourceFileLike): readonly Node[] {
this.assertHasRealPosition("Node without a real position cannot be scanned and thus has no token nodes - use forEachChild and collect the result if that's fine");
return getNodeChildren(this) ?? setNodeChildren(this, createChildren(this, sourceFile));
}
Expand Down Expand Up @@ -473,7 +473,7 @@ class NodeObject<TKind extends SyntaxKind> implements Node {
}
}

function createChildren(node: Node, sourceFile: SourceFileLike | undefined): Node[] {
function createChildren(node: Node, sourceFile: SourceFileLike | undefined): readonly Node[] {
if (!isNodeKind(node.kind)) {
return emptyArray;
}
Expand Down
6 changes: 3 additions & 3 deletions src/services/smartSelection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -279,7 +279,7 @@ function getSelectionChildren(node: Node): readonly Node[] {
* Groups sibling nodes together into their own SyntaxList if they
* a) are adjacent, AND b) match a predicate function.
*/
function groupChildren(children: Node[], groupOn: (child: Node) => boolean): Node[] {
function groupChildren(children: readonly Node[], groupOn: (child: Node) => boolean): Node[] {
const result: Node[] = [];
let group: Node[] | undefined;
for (const child of children) {
Expand Down Expand Up @@ -315,7 +315,7 @@ function groupChildren(children: Node[], groupOn: (child: Node) => boolean): Nod
* @param separateTrailingSemicolon If the last token is a semicolon, it will be returned as a separate
* child rather than be included in the right-hand group.
*/
function splitChildren(children: Node[], pivotOn: (child: Node) => boolean, separateTrailingSemicolon = true): Node[] {
function splitChildren(children: readonly Node[], pivotOn: (child: Node) => boolean, separateTrailingSemicolon = true): readonly Node[] {
if (children.length < 2) {
return children;
}
Expand All @@ -336,7 +336,7 @@ function splitChildren(children: Node[], pivotOn: (child: Node) => boolean, sepa
return separateLastToken ? result.concat(lastToken) : result;
}

function createSyntaxList(children: Node[]): SyntaxList {
function createSyntaxList(children: readonly Node[]): SyntaxList {
Debug.assertGreaterThanOrEqual(children.length, 1);
return setTextRangePosEnd(parseNodeFactory.createSyntaxList(children), children[0].pos, last(children).end);
}
Expand Down
4 changes: 2 additions & 2 deletions src/services/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,9 @@ declare module "../compiler/types" {
getSourceFile(): SourceFile;
getChildCount(sourceFile?: SourceFile): number;
getChildAt(index: number, sourceFile?: SourceFile): Node;
getChildren(sourceFile?: SourceFile): Node[];
getChildren(sourceFile?: SourceFile): readonly Node[];
/** @internal */
getChildren(sourceFile?: SourceFileLike): Node[]; // eslint-disable-line @typescript-eslint/unified-signatures
getChildren(sourceFile?: SourceFileLike): readonly Node[]; // eslint-disable-line @typescript-eslint/unified-signatures
getStart(sourceFile?: SourceFile, includeJsDocComment?: boolean): number;
/** @internal */
getStart(sourceFile?: SourceFileLike, includeJsDocComment?: boolean): number; // eslint-disable-line @typescript-eslint/unified-signatures
Expand Down
2 changes: 1 addition & 1 deletion src/services/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1830,7 +1830,7 @@ function findRightmostToken(n: Node, sourceFile: SourceFileLike): Node | undefin
/**
* Finds the rightmost child to the left of `children[exclusiveStartPosition]` which is a non-all-whitespace token or has constituent tokens.
*/
function findRightmostChildNodeWithTokens(children: Node[], exclusiveStartPosition: number, sourceFile: SourceFileLike, parentKind: SyntaxKind): Node | undefined {
function findRightmostChildNodeWithTokens(children: readonly Node[], exclusiveStartPosition: number, sourceFile: SourceFileLike, parentKind: SyntaxKind): Node | undefined {
for (let i = exclusiveStartPosition - 1; i >= 0; i--) {
const child = children[i];

Expand Down
2 changes: 1 addition & 1 deletion tests/baselines/reference/api/typescript.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4215,7 +4215,7 @@ declare namespace ts {
getSourceFile(): SourceFile;
getChildCount(sourceFile?: SourceFile): number;
getChildAt(index: number, sourceFile?: SourceFile): Node;
getChildren(sourceFile?: SourceFile): Node[];
getChildren(sourceFile?: SourceFile): readonly Node[];
getStart(sourceFile?: SourceFile, includeJsDocComment?: boolean): number;
getFullStart(): number;
getEnd(): number;
Expand Down