From 43345bfddd59b0fd6ef6f08cfd670129460c3150 Mon Sep 17 00:00:00 2001 From: Ivan Goncharov Date: Fri, 17 Sep 2021 11:45:03 +0300 Subject: [PATCH] visitor: speed up visitInParallel by dropping support for unknown AST nodes --- benchmark/visit-benchmark.js | 25 ++++ benchmark/visitInParallel-benchmark.js | 25 ++++ src/index.ts | 1 + src/language/__tests__/visitor-test.ts | 43 ------- src/language/index.ts | 8 +- src/language/visitor.ts | 151 ++++++++++++++++--------- src/utilities/TypeInfo.ts | 6 +- 7 files changed, 157 insertions(+), 102 deletions(-) create mode 100644 benchmark/visit-benchmark.js create mode 100644 benchmark/visitInParallel-benchmark.js diff --git a/benchmark/visit-benchmark.js b/benchmark/visit-benchmark.js new file mode 100644 index 00000000000..ab6a2baac27 --- /dev/null +++ b/benchmark/visit-benchmark.js @@ -0,0 +1,25 @@ +'use strict'; + +const { parse } = require('graphql/language/parser.js'); +const { visit } = require('graphql/language/visitor.js'); + +const { bigSchemaSDL } = require('./fixtures.js'); + +const documentAST = parse(bigSchemaSDL); + +const visitor = { + enter() { + /* do nothing */ + }, + leave() { + /* do nothing */ + }, +}; + +module.exports = { + name: 'Visit all AST nodes', + count: 10, + measure() { + visit(documentAST, visitor); + }, +}; diff --git a/benchmark/visitInParallel-benchmark.js b/benchmark/visitInParallel-benchmark.js new file mode 100644 index 00000000000..cd835dd19ce --- /dev/null +++ b/benchmark/visitInParallel-benchmark.js @@ -0,0 +1,25 @@ +'use strict'; + +const { parse } = require('graphql/language/parser.js'); +const { visit, visitInParallel } = require('graphql/language/visitor.js'); + +const { bigSchemaSDL } = require('./fixtures.js'); + +const documentAST = parse(bigSchemaSDL); + +const visitors = new Array(50).fill({ + enter() { + /* do nothing */ + }, + leave() { + /* do nothing */ + }, +}); + +module.exports = { + name: 'Visit all AST nodes in parallel', + count: 10, + measure() { + visit(documentAST, visitInParallel(visitors)); + }, +}; diff --git a/src/index.ts b/src/index.ts index 836ca22d0d9..c09efb04561 100644 --- a/src/index.ts +++ b/src/index.ts @@ -209,6 +209,7 @@ export { visit, visitInParallel, getVisitFn, + getEnterLeaveForKind, BREAK, Kind, DirectiveLocation, diff --git a/src/language/__tests__/visitor-test.ts b/src/language/__tests__/visitor-test.ts index e63f03ed4a5..903b80d9593 100644 --- a/src/language/__tests__/visitor-test.ts +++ b/src/language/__tests__/visitor-test.ts @@ -420,49 +420,6 @@ describe('Visitor', () => { ]); }); - it('visit nodes with unknown kinds but does not traverse deeper', () => { - const customAST = parse('{ a }'); - // @ts-expect-error - customAST.definitions[0].selectionSet.selections.push({ - kind: 'CustomField', - name: { kind: 'Name', value: 'NamedNodeToBeSkipped' }, - selectionSet: { - kind: 'SelectionSet', - selections: [ - { - kind: 'CustomField', - name: { kind: 'Name', value: 'NamedNodeToBeSkipped' }, - }, - ], - }, - }); - - const visited: Array = []; - visit(customAST, { - enter(node) { - visited.push(['enter', node.kind, getValue(node)]); - }, - leave(node) { - visited.push(['leave', node.kind, getValue(node)]); - }, - }); - - expect(visited).to.deep.equal([ - ['enter', 'Document', undefined], - ['enter', 'OperationDefinition', undefined], - ['enter', 'SelectionSet', undefined], - ['enter', 'Field', undefined], - ['enter', 'Name', 'a'], - ['leave', 'Name', 'a'], - ['leave', 'Field', undefined], - ['enter', 'CustomField', undefined], - ['leave', 'CustomField', undefined], - ['leave', 'SelectionSet', undefined], - ['leave', 'OperationDefinition', undefined], - ['leave', 'Document', undefined], - ]); - }); - it('visits only the specified `Kind` in visitorKeyMap', () => { const visited: Array = []; diff --git a/src/language/index.ts b/src/language/index.ts index 070ca45d234..862896b0749 100644 --- a/src/language/index.ts +++ b/src/language/index.ts @@ -18,7 +18,13 @@ export type { ParseOptions } from './parser'; export { print } from './printer'; -export { visit, visitInParallel, getVisitFn, BREAK } from './visitor'; +export { + visit, + visitInParallel, + getVisitFn, + getEnterLeaveForKind, + BREAK, +} from './visitor'; export type { ASTVisitor, ASTVisitFn, ASTVisitorKeyMap } from './visitor'; export { Location, Token } from './ast'; diff --git a/src/language/visitor.ts b/src/language/visitor.ts index df3081bedd6..7b7b446a4d1 100644 --- a/src/language/visitor.ts +++ b/src/language/visitor.ts @@ -1,8 +1,8 @@ import { inspect } from '../jsutils/inspect'; -import type { Maybe } from '../jsutils/Maybe'; import type { ASTNode, ASTKindToNode } from './ast'; import { isNode } from './ast'; +import { Kind } from './kinds'; /** * A visitor is provided to visit, it contains the collection of @@ -256,6 +256,15 @@ export function visit( visitor: ASTVisitor | ASTReducer, visitorKeys: ASTVisitorKeyMap = QueryDocumentKeys, ): any { + const enterLeaveMap = new Map< + keyof ASTKindToNode, + EnterLeaveVisitor + >(); + + for (const kind of Object.values(Kind)) { + enterLeaveMap.set(kind, getEnterLeaveForKind(visitor, kind)); + } + /* eslint-disable no-undef-init */ let stack: any = undefined; let inArray = Array.isArray(root); @@ -318,29 +327,30 @@ export function visit( if (!isNode(node)) { throw new Error(`Invalid AST Node: ${inspect(node)}.`); } - const visitFn = getVisitFn(visitor, node.kind, isLeaving); - if (visitFn) { - result = visitFn.call(visitor, node, key, parent, path, ancestors); + const visitFn = isLeaving + ? enterLeaveMap.get(node.kind)?.leave + : enterLeaveMap.get(node.kind)?.enter; - if (result === BREAK) { - break; - } + result = visitFn?.call(visitor, node, key, parent, path, ancestors); + + if (result === BREAK) { + break; + } - if (result === false) { - if (!isLeaving) { + if (result === false) { + if (!isLeaving) { + path.pop(); + continue; + } + } else if (result !== undefined) { + edits.push([key, result]); + if (!isLeaving) { + if (isNode(result)) { + node = result; + } else { path.pop(); continue; } - } else if (result !== undefined) { - edits.push([key, result]); - if (!isLeaving) { - if (isNode(result)) { - node = result; - } else { - path.pop(); - continue; - } - } } } } @@ -380,16 +390,31 @@ export function visit( export function visitInParallel( visitors: ReadonlyArray, ): ASTVisitor { - const skipping = new Array(visitors.length); - - return { - enter(...args) { - const node = args[0]; - for (let i = 0; i < visitors.length; i++) { - if (skipping[i] == null) { - const fn = getVisitFn(visitors[i], node.kind, /* isLeaving */ false); - if (fn) { - const result = fn.apply(visitors[i], args); + const skipping = new Array(visitors.length).fill(null); + const mergedVisitor = Object.create(null); + + for (const kind of Object.values(Kind)) { + let hasVisitor = false; + const enterList = new Array(visitors.length).fill(undefined); + const leaveList = new Array(visitors.length).fill(undefined); + + for (let i = 0; i < visitors.length; ++i) { + const { enter, leave } = getEnterLeaveForKind(visitors[i], kind); + hasVisitor ||= enter != null || leave != null; + enterList[i] = enter; + leaveList[i] = leave; + } + + if (!hasVisitor) { + continue; + } + + const mergedEnterLeave: EnterLeaveVisitor = { + enter(...args) { + const node = args[0]; + for (let i = 0; i < visitors.length; i++) { + if (skipping[i] === null) { + const result = enterList[i]?.apply(visitors[i], args); if (result === false) { skipping[i] = node; } else if (result === BREAK) { @@ -399,50 +424,66 @@ export function visitInParallel( } } } - } - }, - leave(...args) { - const node = args[0]; - for (let i = 0; i < visitors.length; i++) { - if (skipping[i] == null) { - const fn = getVisitFn(visitors[i], node.kind, /* isLeaving */ true); - if (fn) { - const result = fn.apply(visitors[i], args); + }, + leave(...args) { + const node = args[0]; + for (let i = 0; i < visitors.length; i++) { + if (skipping[i] === null) { + const result = leaveList[i]?.apply(visitors[i], args); if (result === BREAK) { skipping[i] = BREAK; } else if (result !== undefined && result !== false) { return result; } + } else if (skipping[i] === node) { + skipping[i] = null; } - } else if (skipping[i] === node) { - skipping[i] = null; } - } - }, - }; + }, + }; + + mergedVisitor[kind] = mergedEnterLeave; + } + + return mergedVisitor; } /** - * Given a visitor instance, if it is leaving or not, and a node kind, return - * the function the visitor runtime should call. + * Given a visitor instance and a node kind, return EnterLeaveVisitor for that kind. */ -export function getVisitFn( +export function getEnterLeaveForKind( visitor: ASTVisitor, kind: keyof ASTKindToNode, - isLeaving: boolean, -): Maybe> { +): EnterLeaveVisitor { const kindVisitor: | ASTVisitFn | EnterLeaveVisitor | undefined = (visitor as any)[kind]; - if (kindVisitor) { - if (typeof kindVisitor === 'function') { - // { Kind() {} } - return isLeaving ? undefined : kindVisitor; - } + + if (typeof kindVisitor === 'object') { // { Kind: { enter() {}, leave() {} } } - return isLeaving ? kindVisitor.leave : kindVisitor.enter; + return kindVisitor; + } else if (typeof kindVisitor === 'function') { + // { Kind() {} } + return { enter: kindVisitor, leave: undefined }; } + // { enter() {}, leave() {} } - return isLeaving ? (visitor as any).leave : (visitor as any).enter; + return { enter: (visitor as any).enter, leave: (visitor as any).leave }; +} + +/** + * Given a visitor instance, if it is leaving or not, and a node kind, return + * the function the visitor runtime should call. + * + * @deprecated Please use `getEnterLeaveForKind` instead. Will be removed in v17 + */ +// istanbul ignore next +export function getVisitFn( + visitor: ASTVisitor, + kind: keyof ASTKindToNode, + isLeaving: boolean, +): ASTVisitFn | undefined { + const { enter, leave } = getEnterLeaveForKind(visitor, kind); + return isLeaving ? leave : enter; } diff --git a/src/utilities/TypeInfo.ts b/src/utilities/TypeInfo.ts index e92dfe53cd2..bd0d23971d9 100644 --- a/src/utilities/TypeInfo.ts +++ b/src/utilities/TypeInfo.ts @@ -2,7 +2,7 @@ import type { ASTVisitor } from '../language/visitor'; import type { ASTNode, FieldNode } from '../language/ast'; import { Kind } from '../language/kinds'; import { isNode } from '../language/ast'; -import { getVisitFn } from '../language/visitor'; +import { getEnterLeaveForKind } from '../language/visitor'; import type { Maybe } from '../jsutils/Maybe'; @@ -344,7 +344,7 @@ export function visitWithTypeInfo( enter(...args) { const node = args[0]; typeInfo.enter(node); - const fn = getVisitFn(visitor, node.kind, /* isLeaving */ false); + const fn = getEnterLeaveForKind(visitor, node.kind).enter; if (fn) { const result = fn.apply(visitor, args); if (result !== undefined) { @@ -358,7 +358,7 @@ export function visitWithTypeInfo( }, leave(...args) { const node = args[0]; - const fn = getVisitFn(visitor, node.kind, /* isLeaving */ true); + const fn = getEnterLeaveForKind(visitor, node.kind).leave; let result; if (fn) { result = fn.apply(visitor, args);