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

Traverse performance #10480

Merged
merged 8 commits into from Nov 5, 2019
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/babel-traverse/src/index.js
Expand Up @@ -31,6 +31,10 @@ export default function traverse(
}
}

if (!t.VISITOR_KEYS[parent.type]) {
return;
JLHwung marked this conversation as resolved.
Show resolved Hide resolved
}

visitors.explode(opts);

traverse.node(parent, opts, scope, state, parentPath);
Expand Down
22 changes: 13 additions & 9 deletions packages/babel-traverse/src/path/context.js
@@ -1,6 +1,7 @@
// This file contains methods responsible for maintaining a TraversalContext.

import traverse from "../index";
import { SHOULD_SKIP, SHOULD_STOP } from "./index";

export function call(key): boolean {
const opts = this.opts;
Expand Down Expand Up @@ -43,7 +44,8 @@ export function _call(fns?: Array<Function>): boolean {
// node has been replaced, it will have been requeued
if (this.node !== node) return true;

if (this.shouldStop || this.shouldSkip || this.removed) return true;
// this.shouldSkip || this.shouldStop || this.removed
if (this._traverseFlags > 0) return true;
}

return false;
Expand Down Expand Up @@ -92,12 +94,15 @@ export function skip() {
}

export function skipKey(key) {
if (this.skipKeys == null) {
this.skipKeys = {};
}
Copy link
Member

Choose a reason for hiding this comment

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

I wish https://github.com/tc39/proposal-logical-assignment wasn't just stage 1 😛

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Offtopic: we still need a PR to add nullish coalescing and optional chaining.

Copy link
Member

@hzoo hzoo Sep 24, 2019

Choose a reason for hiding this comment

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

We can always use new stage features if you are willing to move off of them if they change (I think that one is small enough that it's fine) 😁

this.skipKeys[key] = true;
}

export function stop() {
this.shouldStop = true;
this.shouldSkip = true;
// this.shouldSkip = true; this.shouldStop = true;
this._traverseFlags |= SHOULD_SKIP | SHOULD_STOP;
}

export function setScope() {
Expand All @@ -117,10 +122,11 @@ export function setScope() {
}

export function setContext(context) {
this.shouldSkip = false;
this.shouldStop = false;
this.removed = false;
this.skipKeys = {};
if (this.skipKeys != null) {
this.skipKeys = {};
}
// this.shouldSkip = false; this.shouldStop = false; this.removed = false;
this._traverseFlags = 0;

if (context) {
this.context = context;
Expand Down Expand Up @@ -215,9 +221,7 @@ export function pushContext(context) {
}

export function setup(parentPath, container, listKey, key) {
this.inList = !!listKey;
this.listKey = listKey;
this.parentKey = listKey || key;
this.container = container;

this.parentPath = parentPath || this.parentPath;
Expand Down
67 changes: 57 additions & 10 deletions packages/babel-traverse/src/path/index.js
Expand Up @@ -23,29 +23,29 @@ import * as NodePath_comments from "./comments";

const debug = buildDebug("babel");

export const REMOVED = 1 << 0;
export const SHOULD_STOP = 1 << 1;
export const SHOULD_SKIP = 1 << 2;
Comment on lines +26 to +28
Copy link
Member

Choose a reason for hiding this comment

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

Some great work doing this research!

I would just like to note that using bitwise flags and operators is for sure going to make contributing and reading the coding that much more difficult (for me, and I will assume some others). I think I mentioned it earlier on Slack and it's a hard call but trading off readability/performance. I don't know what else we can do other than writing comments to explain things though


export default class NodePath {
constructor(hub: HubInterface, parent: Object) {
this.parent = parent;
this.hub = hub;
this.contexts = [];
this.data = Object.create(null);
this.shouldSkip = false;
this.shouldStop = false;
this.removed = false;
this.data = null;
// this.shouldSkip = false; this.shouldStop = false; this.removed = false;
this._traverseFlags = 0;
this.state = null;
this.opts = null;
this.skipKeys = null;
this.parentPath = null;
this.context = null;
this.container = null;
this.listKey = null;
this.inList = false;
this.parentKey = null;
this.key = null;
this.node = null;
this.scope = null;
this.type = null;
this.typeAnnotation = null;
}

parent: Object;
Expand All @@ -57,18 +57,16 @@ export default class NodePath {
removed: boolean;
state: any;
opts: ?Object;
_traverseFlags: number;
skipKeys: ?Object;
parentPath: ?NodePath;
context: TraversalContext;
container: ?Object | Array<Object>;
listKey: ?string;
inList: boolean;
parentKey: ?string;
key: ?string;
node: ?Object;
scope: Scope;
type: ?string;
typeAnnotation: ?Object;

static get({ hub, parentPath, parent, container, listKey, key }): NodePath {
if (!hub && parentPath) {
Expand Down Expand Up @@ -111,10 +109,16 @@ export default class NodePath {
}

setData(key: string, val: any): any {
if (this.data == null) {
this.data = Object.create(null);
}
return (this.data[key] = val);
}

getData(key: string, def?: any): any {
if (this.data == null) {
this.data = Object.create(null);
}
let val = this.data[key];
if (val === undefined && def !== undefined) val = this.data[key] = def;
return val;
Expand Down Expand Up @@ -152,6 +156,49 @@ export default class NodePath {
toString() {
return generator(this.node).code;
}

get inList() {
return !!this.listKey;
}

get parentKey() {
return this.listKey || this.key;
}

get shouldSkip() {
return !!(this._traverseFlags & SHOULD_SKIP);
}

set shouldSkip(v) {
if (v) {
this._traverseFlags |= SHOULD_SKIP;
} else {
this._traverseFlags &= ~SHOULD_SKIP;
}
}

get shouldStop() {
return !!(this._traverseFlags & SHOULD_STOP);
}

set shouldStop(v) {
if (v) {
this._traverseFlags |= SHOULD_STOP;
} else {
this._traverseFlags &= ~SHOULD_STOP;
}
}

get removed() {
return !!(this._traverseFlags & REMOVED);
}
set removed(v) {
if (v) {
this._traverseFlags |= REMOVED;
} else {
this._traverseFlags &= ~REMOVED;
}
}
}

Object.assign(
Expand Down
5 changes: 3 additions & 2 deletions packages/babel-traverse/src/path/removal.js
@@ -1,6 +1,7 @@
// This file contains methods responsible for removing a node.

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

export function remove() {
this._assertUnremoved();
Expand Down Expand Up @@ -39,8 +40,8 @@ export function _remove() {
}

export function _markRemoved() {
this.shouldSkip = true;
this.removed = true;
// this.shouldSkip = true; this.removed = true;
this._traverseFlags |= SHOULD_SKIP | REMOVED;
this.node = null;
}

Expand Down