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

Reduce linear search on list traversing #12302

Merged
merged 2 commits into from Nov 5, 2020
Merged

Reduce linear search on list traversing #12302

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
af2a53e
perf: reduce linear search on list traversing
JLHwung Nov 3, 2020
3a365bf
fix: remove parent cache when replacing node
JLHwung Nov 3, 2020
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
7 changes: 4 additions & 3 deletions packages/babel-traverse/src/context.js
View file
Open in desktop
Expand Up @@ -95,7 +95,7 @@ export default class TraversalContext {
this.queue = queue;
this.priorityQueue = [];

const visited = [];
const visited = new WeakSet();
let stop = false;

// visit the queue
Expand All @@ -120,8 +120,9 @@ export default class TraversalContext {
}

// ensure we don't visit the same node twice
if (visited.indexOf(path.node) >= 0) continue;
visited.push(path.node);
const { node } = path;
if (visited.has(node)) continue;
if (node) visited.add(node);
Copy link
Contributor Author

@JLHwung JLHwung Nov 4, 2020 •
edited

Choose a reason for hiding this comment

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

Note that when path.node is null, the current behaviour is to visit this path once but skip other nullish node paths since null can be hold in array. In this PR they will all be visited. This could be a breaking change in a way but I think the equality of two paths whose path.node is null, is not well-defined.

The usage of WeakSet is intentional, so we don't leak memory when path.node is GCed.


if (path.visit()) {
stop = true;
Expand Down
18 changes: 5 additions & 13 deletions packages/babel-traverse/src/path/index.js
View file
Open in desktop
Expand Up @@ -67,24 +67,16 @@ export default class NodePath {

const targetNode = container[key];

const paths = pathCache.get(parent) || [];
if (!pathCache.has(parent)) {
let paths = pathCache.get(parent);
if (!paths) {
paths = new Map();
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't use a WeakMap here because we iterate on pathCache in https://github.com/babel/babel/pull/12302/files#diff-e406da86b11f8d4846c6ffb3bf527e1c0fa230888127f90bb8978c16a9bd025aR166

Copy link
Member

Choose a reason for hiding this comment

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

We can create an IterableWeakMap, falling back to a normal Map on Node.js <13

nodejs/node#35915

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh the IterableWeakMap is not a builtin object nor supposed to be a public Node.js API, I suggest we hold off a bit since WeakRef (relied by IterableWeakMap) is still stage 3 and @babel/traverse is heavily used.

pathCache.set(parent, paths);
}

let path;

for (let i = 0; i < paths.length; i++) {
const pathCheck = paths[i];
if (pathCheck.node === targetNode) {
path = pathCheck;
break;
}
}

let path = paths.get(targetNode);
if (!path) {
path = new NodePath(hub, parent);
paths.push(path);
if (targetNode) paths.set(targetNode, path);
}

path.setup(parentPath, container, listKey, key);
Expand Down
3 changes: 1 addition & 2 deletions packages/babel-traverse/src/path/modification.js
View file
Open in desktop
Expand Up @@ -163,8 +163,7 @@ export function updateSiblingKeys(fromIndex, incrementBy) {
if (!this.parent) return;

const paths = pathCache.get(this.parent);
for (let i = 0; i < paths.length; i++) {
const path = paths[i];
for (const [, path] of paths) {
if (path.key >= fromIndex) {
path.key += incrementBy;
}
Expand Down
2 changes: 2 additions & 0 deletions packages/babel-traverse/src/path/removal.js
View file
Open in desktop
@@ -1,6 +1,7 @@
// This file contains methods responsible for removing a node.

import { hooks } from "./lib/removal-hooks";
import { path as pathCache } from "../cache";
import { REMOVED, SHOULD_SKIP } from "./index";

export function remove() {
Expand Down Expand Up @@ -44,6 +45,7 @@ export function _remove() {
export function _markRemoved() {
// this.shouldSkip = true; this.removed = true;
this._traverseFlags |= SHOULD_SKIP | REMOVED;
if (this.parent) pathCache.get(this.parent).delete(this.node);
this.node = null;
}

Expand Down
2 changes: 2 additions & 0 deletions packages/babel-traverse/src/path/replacement.js
View file
Open in desktop
Expand Up @@ -3,6 +3,7 @@
import { codeFrameColumns } from "@babel/code-frame";
import traverse from "../index";
import NodePath from "./index";
import { path as pathCache } from "../cache";
import { parse } from "@babel/parser";
import * as t from "@babel/types";

Expand Down Expand Up @@ -49,6 +50,7 @@ export function replaceWithMultiple(nodes: Array<Object>) {
nodes = this._verifyNodeList(nodes);
t.inheritLeadingComments(nodes[0], this.node);
t.inheritTrailingComments(nodes[nodes.length - 1], this.node);
pathCache.get(this.parent).delete(this.node);
this.node = this.container[this.key] = null;
const paths = this.insertAfter(nodes);

Expand Down