Skip to content

Create a SourceFile-level indirection on children maps, store SyntaxList children directly on nodes. #59154

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

Merged
merged 4 commits into from
Jul 8, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
52 changes: 44 additions & 8 deletions src/compiler/factory/nodeChildren.ts
Original file line number Diff line number Diff line change
@@ -1,25 +1,61 @@
import {
Debug,
emptyArray,
isNodeKind,
Node,
SourceFileLike,
SyntaxKind,
SyntaxList,
} from "../_namespaces/ts.js";

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

/** @internal */
export function getNodeChildren(node: Node): readonly Node[] | undefined {
if (!isNodeKind(node.kind)) return emptyArray;
export function getNodeChildren(node: Node, sourceFile: SourceFileLike): readonly Node[] | undefined {
const kind = node.kind;
if (!isNodeKind(kind)) {
return emptyArray;
}
if (kind === SyntaxKind.SyntaxList) {
return (node as SyntaxList)._children;
}

return nodeChildren.get(node);
return sourceFileToNodeChildren.get(sourceFile)?.get(node);
}

/** @internal */
export function setNodeChildren(node: Node, children: readonly Node[]): readonly Node[] {
nodeChildren.set(node, children);
export function setNodeChildren(node: Node, sourceFile: SourceFileLike, children: readonly Node[]): readonly Node[] {
if (node.kind === SyntaxKind.SyntaxList) {
// SyntaxList children are always eagerly created in the process of
// creating their parent's `children` list. We shouldn't need to set them here.
Debug.fail("Should not need to re-set the children of a SyntaxList.");
}

let map = sourceFileToNodeChildren.get(sourceFile);
if (map === undefined) {
map = new WeakMap();
sourceFileToNodeChildren.set(sourceFile, map);
}
map.set(node, children);
return children;
}

/** @internal */
export function unsetNodeChildren(node: Node) {
nodeChildren.delete(node);
export function unsetNodeChildren(node: Node, origSourceFile: SourceFileLike) {
if (node.kind === SyntaxKind.SyntaxList) {
// Syntax lists are synthesized and we store their children directly on them.
// They are a special case where we expect incremental parsing to toss them away entirely
// if a change intersects with their containing parents.
Debug.fail("Did not expect to unset the children of a SyntaxList.");
}
sourceFileToNodeChildren.get(origSourceFile)?.delete(node);
}

/** @internal */
export function transferSourceFileChildren(sourceFile: SourceFileLike, targetSourceFile: SourceFileLike) {
const map = sourceFileToNodeChildren.get(sourceFile);
if (map !== undefined) {
sourceFileToNodeChildren.delete(sourceFile);
Copy link
Member

Choose a reason for hiding this comment

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

Is it harmful to leave the old entry behind? Theoretically it will get GC'd once the old one goes away, but I guess maybe it's bad to have two people able to write to this? (Theoretically their keys would be distinct anyway?)

Copy link
Member Author

@DanielRosenwasser DanielRosenwasser Jul 8, 2024

Choose a reason for hiding this comment

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

That's not a bad reason, but I guess the main thing I was thinking about is avoiding a set resizing the table if possible. It's probably a small cost, and not bullet-proof, but there's no reason for the two SourceFiles to take up space in the map if they're not being used.

Technically you could argue we never need to call unsetNodeChildren either since the node will never be reused and eventually go away. I think it's better to remove them if possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since WeakMap is not iterable and therefore doesn't have a defined order of keys, I'm unclear on whether or not deleting entries from a WeakMap is a trivial operation. It's an interesting question which way performance goes on trimming a WeakMap vs. letting the values get GC'd, but there is something to be said for cleanliness.

sourceFileToNodeChildren.set(targetSourceFile, map);
}
}
3 changes: 1 addition & 2 deletions src/compiler/factory/nodeFactory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -388,7 +388,6 @@ import {
setEmitFlags,
setIdentifierAutoGenerate,
setIdentifierTypeArguments,
setNodeChildren,
setParent,
setTextRange,
ShorthandPropertyAssignment,
Expand Down Expand Up @@ -6212,7 +6211,7 @@ export function createNodeFactory(flags: NodeFactoryFlags, baseFactory: BaseNode
// @api
function createSyntaxList(children: readonly Node[]) {
const node = createBaseNode<SyntaxList>(SyntaxKind.SyntaxList);
setNodeChildren(node, children);
node._children = children;
return node;
}

Expand Down
16 changes: 9 additions & 7 deletions src/compiler/parser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -369,6 +369,7 @@ import {
tokenIsIdentifierOrKeywordOrGreaterThan,
tokenToString,
tracing,
transferSourceFileChildren,
TransformFlags,
TryStatement,
TupleTypeNode,
Expand Down Expand Up @@ -9969,6 +9970,7 @@ namespace IncrementalParser {
aggressiveChecks,
);
result.impliedNodeFormat = sourceFile.impliedNodeFormat;
transferSourceFileChildren(sourceFile, result);
return result;
}

Expand Down Expand Up @@ -10021,9 +10023,9 @@ namespace IncrementalParser {
}
}

function moveElementEntirelyPastChangeRange(element: Node, isArray: false, delta: number, oldText: string, newText: string, aggressiveChecks: boolean): void;
function moveElementEntirelyPastChangeRange(element: NodeArray<Node>, isArray: true, delta: number, oldText: string, newText: string, aggressiveChecks: boolean): void;
function moveElementEntirelyPastChangeRange(element: Node | NodeArray<Node>, isArray: boolean, delta: number, oldText: string, newText: string, aggressiveChecks: boolean) {
function moveElementEntirelyPastChangeRange(element: Node, origSourceFile: SourceFile, isArray: false, delta: number, oldText: string, newText: string, aggressiveChecks: boolean): void;
function moveElementEntirelyPastChangeRange(element: NodeArray<Node>, origSourceFile: SourceFile, isArray: true, delta: number, oldText: string, newText: string, aggressiveChecks: boolean): void;
function moveElementEntirelyPastChangeRange(element: Node | NodeArray<Node>, origSourceFile: SourceFile, isArray: boolean, delta: number, oldText: string, newText: string, aggressiveChecks: boolean) {
if (isArray) {
visitArray(element as NodeArray<Node>);
}
Expand All @@ -10040,7 +10042,7 @@ namespace IncrementalParser {

// Ditch any existing LS children we may have created. This way we can avoid
// moving them forward.
unsetNodeChildren(node);
unsetNodeChildren(node, origSourceFile);

setTextRangePosEnd(node, node.pos + delta, node.end + delta);

Expand Down Expand Up @@ -10187,7 +10189,7 @@ namespace IncrementalParser {
if (child.pos > changeRangeOldEnd) {
// Node is entirely past the change range. We need to move both its pos and
// end, forward or backward appropriately.
moveElementEntirelyPastChangeRange(child, /*isArray*/ false, delta, oldText, newText, aggressiveChecks);
moveElementEntirelyPastChangeRange(child, sourceFile, /*isArray*/ false, delta, oldText, newText, aggressiveChecks);
return;
}

Expand All @@ -10197,7 +10199,7 @@ namespace IncrementalParser {
const fullEnd = child.end;
if (fullEnd >= changeStart) {
markAsIntersectingIncrementalChange(child);
unsetNodeChildren(child);
unsetNodeChildren(child, sourceFile);

// Adjust the pos or end (or both) of the intersecting element accordingly.
adjustIntersectingElement(child, changeStart, changeRangeOldEnd, changeRangeNewEnd, delta);
Expand All @@ -10220,7 +10222,7 @@ namespace IncrementalParser {
if (array.pos > changeRangeOldEnd) {
// Array is entirely after the change range. We need to move it, and move any of
// its children.
moveElementEntirelyPastChangeRange(array, /*isArray*/ true, delta, oldText, newText, aggressiveChecks);
moveElementEntirelyPastChangeRange(array, sourceFile, /*isArray*/ true, delta, oldText, newText, aggressiveChecks);
return;
}

Expand Down
6 changes: 6 additions & 0 deletions src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9816,6 +9816,12 @@ export interface DiagnosticCollection {
// SyntaxKind.SyntaxList
export interface SyntaxList extends Node {
kind: SyntaxKind.SyntaxList;

// Unlike other nodes which may or may not have their child nodes calculated,
// the entire purpose of a SyntaxList is to hold child nodes.
// Instead of using the WeakMap machinery in `nodeChildren.ts`,
// we just store the children directly on the SyntaxList.
/** @internal */ _children: readonly Node[];
}

// dprint-ignore
Expand Down
7 changes: 4 additions & 3 deletions src/compiler/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1187,7 +1187,7 @@ export function getTokenPosOfNode(node: Node, sourceFile?: SourceFileLike, inclu

if (isJSDocNode(node) || node.kind === SyntaxKind.JsxText) {
// JsxText cannot actually contain comments, even though the scanner will think it sees comments
return skipTrivia((sourceFile || getSourceFileOfNode(node)).text, node.pos, /*stopAfterLineBreak*/ false, /*stopAtComments*/ true);
return skipTrivia((sourceFile ?? getSourceFileOfNode(node)).text, node.pos, /*stopAfterLineBreak*/ false, /*stopAtComments*/ true);
}

if (includeJsDoc && hasJSDocNodes(node)) {
Expand All @@ -1199,14 +1199,15 @@ export function getTokenPosOfNode(node: Node, sourceFile?: SourceFileLike, inclu
// trivia for the list, we may have skipped the JSDocComment as well. So we should process its
// first child to determine the actual position of its first token.
if (node.kind === SyntaxKind.SyntaxList) {
const first = firstOrUndefined(getNodeChildren(node));
sourceFile ??= getSourceFileOfNode(node);
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this wasted work when the getNodeChildren function special cases SyntaxKind.SyntaxList to not use sourceFile?

Copy link
Member Author

@DanielRosenwasser DanielRosenwasser Jul 8, 2024

Choose a reason for hiding this comment

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

Apparently not - this is a recursive function, and at almost every other terminal branch the source file is consulted for something. So I can remove the check, but in practice we'll pull on getSourceFileOfNode at some point lower in the tree.

Happy to remove it if you all think it's cleaner to just get the _children property.

const first = firstOrUndefined(getNodeChildren(node, sourceFile));
if (first) {
return getTokenPosOfNode(first, sourceFile, includeJsDoc);
}
}

return skipTrivia(
(sourceFile || getSourceFileOfNode(node)).text,
(sourceFile ?? getSourceFileOfNode(node)).text,
node.pos,
/*stopAfterLineBreak*/ false,
/*stopAtComments*/ false,
Expand Down
6 changes: 3 additions & 3 deletions src/services/services.ts
Original file line number Diff line number Diff line change
Expand Up @@ -457,9 +457,9 @@ class NodeObject<TKind extends SyntaxKind> implements Node {
return this.getChildren(sourceFile)[index];
}

public getChildren(sourceFile?: SourceFileLike): readonly Node[] {
public getChildren(sourceFile: SourceFileLike = getSourceFileOfNode(this)): 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));
return getNodeChildren(this, sourceFile) ?? setNodeChildren(this, sourceFile, createChildren(this, sourceFile));
}

public getFirstToken(sourceFile?: SourceFileLike): Node | undefined {
Expand Down Expand Up @@ -558,7 +558,7 @@ function createSyntaxList(nodes: NodeArray<Node>, parent: Node): Node {
pos = node.end;
}
addSyntheticNodes(children, pos, nodes.end, parent);
setNodeChildren(list, children);
list._children = children;
return list;
}

Expand Down