diff --git a/packages/core/core/src/BundleGraph.js b/packages/core/core/src/BundleGraph.js index a4a9122056c..350a6bf73f8 100644 --- a/packages/core/core/src/BundleGraph.js +++ b/packages/core/core/src/BundleGraph.js @@ -1386,6 +1386,10 @@ export default class BundleGraph { }); } + /** + * TODO: Document why this works like this & why visitor order matters + * on these use-cases. + */ traverseBundle( bundle: Bundle, visit: GraphVisitor, diff --git a/packages/core/core/test/test-utils.js b/packages/core/core/test/test-utils.js index b44e22be99f..6a430bb4f3d 100644 --- a/packages/core/core/test/test-utils.js +++ b/packages/core/core/test/test-utils.js @@ -54,6 +54,7 @@ export const DEFAULT_OPTIONS: ParcelOptions = { featureFlags: { exampleFeature: false, configKeyInvalidation: false, + dfsFasterRefactor: false, }, }; diff --git a/packages/core/feature-flags/src/index.js b/packages/core/feature-flags/src/index.js index c66b372d0dd..5835b8cb686 100644 --- a/packages/core/feature-flags/src/index.js +++ b/packages/core/feature-flags/src/index.js @@ -8,6 +8,7 @@ export type FeatureFlags = _FeatureFlags; export const DEFAULT_FEATURE_FLAGS: FeatureFlags = { exampleFeature: false, configKeyInvalidation: false, + dfsFasterRefactor: false, }; let featureFlagValues: FeatureFlags = {...DEFAULT_FEATURE_FLAGS}; diff --git a/packages/core/feature-flags/src/types.js b/packages/core/feature-flags/src/types.js index 4edb1cf42f0..58250a4a62b 100644 --- a/packages/core/feature-flags/src/types.js +++ b/packages/core/feature-flags/src/types.js @@ -9,4 +9,8 @@ export type FeatureFlags = {| * `config.getConfigFrom(..., {packageKey: '...'})` and the value itself hasn't changed. */ +configKeyInvalidation: boolean, + /** + * Refactors dfsNew to use an iterative approach. + */ + +dfsFasterRefactor: boolean, |}; diff --git a/packages/core/graph/package.json b/packages/core/graph/package.json index 049030cd6f1..98a321076a7 100644 --- a/packages/core/graph/package.json +++ b/packages/core/graph/package.json @@ -20,6 +20,7 @@ "node": ">= 16.0.0" }, "dependencies": { + "@parcel/feature-flags": "2.12.0", "nullthrows": "^1.1.1" } } diff --git a/packages/core/graph/src/AdjacencyList.js b/packages/core/graph/src/AdjacencyList.js index 6d88084122a..8d9e6229e57 100644 --- a/packages/core/graph/src/AdjacencyList.js +++ b/packages/core/graph/src/AdjacencyList.js @@ -30,7 +30,7 @@ export type AdjacencyListOptions = {| minGrowFactor?: number, /** The size after which to grow the capacity by the minimum factor. */ peakCapacity?: number, - /** The percentage of deleted edges above which the capcity should shink. */ + /** The percentage of deleted edges above which the capacity should shrink. */ unloadFactor?: number, /** The amount by which to shrink the capacity. */ shrinkFactor?: number, @@ -328,7 +328,7 @@ export default class AdjacencyList { * * Note that this method does not increment the node count * (that only happens in `addEdge`), it _may_ preemptively resize - * the nodes array if it is at capacity, under the asumption that + * the nodes array if it is at capacity, under the assumption that * at least 1 edge to or from this new node will be added. * * Returns the id of the added node. diff --git a/packages/core/graph/src/Graph.js b/packages/core/graph/src/Graph.js index 11072cb7cc9..466e06bc13b 100644 --- a/packages/core/graph/src/Graph.js +++ b/packages/core/graph/src/Graph.js @@ -3,6 +3,7 @@ import {fromNodeId} from './types'; import AdjacencyList, {type SerializedAdjacencyList} from './AdjacencyList'; import type {Edge, NodeId} from './types'; +import {getFeatureFlag} from '@parcel/feature-flags'; import type { TraversalActions, GraphVisitor, @@ -28,6 +29,51 @@ export type SerializedGraph = {| export type AllEdgeTypes = -1; export const ALL_EDGE_TYPES: AllEdgeTypes = -1; +type DFSCommandVisit = {| + nodeId: NodeId, + context: TContext | null, +|}; + +type DFSCommandExit = {| + nodeId: NodeId, + exit: GraphTraversalCallback, + context: TContext | null, +|}; + +/** + * Internal type used for queue iterative DFS implementation. + */ +type DFSCommand = + | DFSCommandVisit + | DFSCommandExit; + +/** + * Options for DFS traversal. + */ +export type DFSParams = {| + visit: GraphVisitor, + /** + * Custom function to get next entries to visit. + * + * This can be a performance bottleneck as arrays are created on every node visit. + * + * @deprecated This will be replaced by a static `traversalType` set of orders in the future + * + * Currently, this is only used in 3 ways: + * + * - Traversing down the tree (normal DFS) + * - Traversing up the tree (ancestors) + * - Filtered version of traversal; which does not need to exist at the DFS level as the visitor + * can handle filtering + * - Sorted traversal of BundleGraph entries, which does not have a clear use-case, but may + * not be safe to remove + * + * Only due to the latter we aren't replacing this. + */ + getChildren: (nodeId: NodeId) => Array, + startNodeId?: ?NodeId, +|}; + export default class Graph { nodes: Array; adjacencyList: AdjacencyList; @@ -289,11 +335,17 @@ export default class Graph { ) { return this.dfsFast(enter, startNodeId); } else { - return this.dfs({ - visit, - startNodeId, - getChildren: nodeId => this.getNodeIdsConnectedFrom(nodeId, type), - }); + return getFeatureFlag('dfsFasterRefactor') + ? this.dfsNew({ + visit, + startNodeId, + getChildren: nodeId => this.getNodeIdsConnectedFrom(nodeId, type), + }) + : this.dfs({ + visit, + startNodeId, + getChildren: nodeId => this.getNodeIdsConnectedFrom(nodeId, type), + }); } } @@ -449,15 +501,122 @@ export default class Graph { return; } + /** + * Iterative implementation of DFS that supports all use-cases. + * + * This replaces `dfs` and will replace `dfsFast`. + */ + dfsNew({ + visit, + startNodeId, + getChildren, + }: DFSParams): ?TContext { + let traversalStartNode = nullthrows( + startNodeId ?? this.rootNodeId, + 'A start node is required to traverse', + ); + this._assertHasNodeId(traversalStartNode); + + let visited; + if (!this._visited || this._visited.capacity < this.nodes.length) { + this._visited = new BitSet(this.nodes.length); + visited = this._visited; + } else { + visited = this._visited; + visited.clear(); + } + // Take shared instance to avoid re-entrancy issues. + this._visited = null; + + let stopped = false; + let skipped = false; + let actions: TraversalActions = { + skipChildren() { + skipped = true; + }, + stop() { + stopped = true; + }, + }; + + const queue: DFSCommand[] = [ + {nodeId: traversalStartNode, context: null}, + ]; + const enter = typeof visit === 'function' ? visit : visit.enter; + while (queue.length !== 0) { + const command = queue.pop(); + + if (command.exit != null) { + let {nodeId, context, exit} = command; + let newContext = exit(nodeId, command.context, actions); + if (typeof newContext !== 'undefined') { + // $FlowFixMe[reassign-const] + context = newContext; + } + + if (skipped) { + continue; + } + + if (stopped) { + this._visited = visited; + return context; + } + } else { + let {nodeId, context} = command; + if (!this.hasNode(nodeId) || visited.has(nodeId)) continue; + visited.add(nodeId); + + skipped = false; + if (enter) { + let newContext = enter(nodeId, context, actions); + if (typeof newContext !== 'undefined') { + // $FlowFixMe[reassign-const] + context = newContext; + } + } + + if (skipped) { + continue; + } + + if (stopped) { + this._visited = visited; + return context; + } + + if (typeof visit !== 'function' && visit.exit) { + queue.push({ + nodeId, + exit: visit.exit, + context, + }); + } + + // TODO turn into generator function + const children = getChildren(nodeId); + for (let i = children.length - 1; i > -1; i -= 1) { + const child = children[i]; + if (visited.has(child)) { + continue; + } + + queue.push({nodeId: child, context}); + } + } + } + + this._visited = visited; + } + + /** + * @deprecated Will be replaced by `dfsNew` + */ dfs({ visit, startNodeId, getChildren, - }: {| - visit: GraphVisitor, - getChildren(nodeId: NodeId): Array, - startNodeId?: ?NodeId, - |}): ?TContext { + }: DFSParams): ?TContext { let traversalStartNode = nullthrows( startNodeId ?? this.rootNodeId, 'A start node is required to traverse', diff --git a/packages/core/graph/test/Graph.test.js b/packages/core/graph/test/Graph.test.js index faf9cbc27e8..8a5eec586ab 100644 --- a/packages/core/graph/test/Graph.test.js +++ b/packages/core/graph/test/Graph.test.js @@ -2,9 +2,10 @@ import assert from 'assert'; import sinon from 'sinon'; +import type {TraversalActions} from '@parcel/types-internal'; -import Graph from '../src/Graph'; -import {toNodeId} from '../src/types'; +import Graph, {type DFSParams} from '../src/Graph'; +import {toNodeId, type NodeId} from '../src/types'; describe('Graph', () => { it('constructor should initialize an empty graph', () => { @@ -340,4 +341,229 @@ describe('Graph', () => { assert.deepEqual(graph.nodes.filter(Boolean), ['root']); assert.deepStrictEqual(Array.from(graph.getAllEdges()), []); }); + + describe('dfs(...)', () => { + function testSuite( + name: string, + dfsImpl: (graph: Graph, DFSParams) => mixed | null | void, + ) { + it(`${name} throws if the graph is empty`, () => { + const graph = new Graph(); + const visit = sinon.stub(); + const getChildren = sinon.stub(); + assert.throws(() => { + dfsImpl(graph, { + visit, + startNodeId: 0, + getChildren, + }); + }, /Does not have node 0/); + }); + + it(`${name} visits a single node`, () => { + const graph = new Graph(); + graph.addNode('root'); + const visit = sinon.stub(); + const getChildren = () => []; + dfsImpl(graph, { + visit, + startNodeId: 0, + getChildren, + }); + + assert(visit.calledOnce); + }); + + it(`${name} visits all connected nodes in DFS order`, () => { + const graph = new Graph(); + graph.addNode('0'); + graph.addNode('1'); + graph.addNode('2'); + graph.addNode('3'); + graph.addNode('disconnected-1'); + graph.addNode('disconnected-2'); + graph.addEdge(0, 1); + graph.addEdge(0, 2); + graph.addEdge(1, 3); + graph.addEdge(2, 3); + + const order = []; + const visit = (node: NodeId) => { + order.push(node); + }; + const getChildren = (node: NodeId) => + graph.getNodeIdsConnectedFrom(node); + dfsImpl(graph, { + visit, + startNodeId: 0, + getChildren, + }); + + assert.deepEqual(order, [0, 1, 3, 2]); + }); + + describe(`${name} actions tests`, () => { + it(`${name} skips children if skip is called on a node`, () => { + const graph = new Graph(); + graph.addNode('0'); + graph.addNode('1'); + graph.addNode('2'); + graph.addNode('3'); + graph.addNode('disconnected-1'); + graph.addNode('disconnected-2'); + graph.addEdge(0, 1); + graph.addEdge(1, 2); + graph.addEdge(0, 3); + + const order = []; + const visit = ( + node: NodeId, + context: mixed | null, + actions: TraversalActions, + ) => { + if (node === 1) actions.skipChildren(); + order.push(node); + }; + const getChildren = (node: NodeId) => + graph.getNodeIdsConnectedFrom(node); + dfsImpl(graph, { + visit, + startNodeId: 0, + getChildren, + }); + + assert.deepEqual(order, [0, 1, 3]); + }); + + it(`${name} stops the traversal if stop is called`, () => { + const graph = new Graph(); + graph.addNode('0'); + graph.addNode('1'); + graph.addNode('2'); + graph.addNode('3'); + graph.addNode('disconnected-1'); + graph.addNode('disconnected-2'); + graph.addEdge(0, 1); + graph.addEdge(1, 2); + graph.addEdge(1, 3); + graph.addEdge(0, 2); + graph.addEdge(2, 3); + + const order = []; + const visit = ( + node: NodeId, + context: mixed | null, + actions: TraversalActions, + ) => { + order.push(node); + if (node === 1) { + actions.stop(); + return 'result'; + } + return 'other'; + }; + const getChildren = (node: NodeId) => + graph.getNodeIdsConnectedFrom(node); + const result = dfsImpl(graph, { + visit, + startNodeId: 0, + getChildren, + }); + + assert.deepEqual(order, [0, 1]); + assert.equal(result, 'result'); + }); + }); + + describe(`${name} context tests`, () => { + it(`${name} passes the context between visitors`, () => { + const graph = new Graph(); + graph.addNode('0'); + graph.addNode('1'); + graph.addNode('2'); + graph.addNode('3'); + graph.addNode('disconnected-1'); + graph.addNode('disconnected-2'); + graph.addEdge(0, 1); + graph.addEdge(1, 2); + graph.addEdge(1, 3); + graph.addEdge(0, 2); + graph.addEdge(2, 3); + + const contexts = []; + const visit = (node: NodeId, context: mixed | null) => { + contexts.push([node, context]); + return `node-${node}-created-context`; + }; + const getChildren = (node: NodeId) => + graph.getNodeIdsConnectedFrom(node); + const result = dfsImpl(graph, { + visit, + startNodeId: 0, + getChildren, + }); + + assert.deepEqual(contexts, [ + [0, undefined], + [1, 'node-0-created-context'], + [2, 'node-1-created-context'], + [3, 'node-2-created-context'], + ]); + assert.equal(result, undefined); + }); + }); + + describe(`${name} exit visitor tests`, () => { + it(`${name} calls the exit visitor`, () => { + const graph = new Graph(); + graph.addNode('0'); + graph.addNode('1'); + graph.addNode('2'); + graph.addNode('3'); + graph.addNode('disconnected-1'); + graph.addNode('disconnected-2'); + graph.addEdge(0, 1); + graph.addEdge(1, 2); + graph.addEdge(1, 3); + graph.addEdge(0, 2); + + const contexts = []; + const visit = (node: NodeId, context: mixed | null) => { + contexts.push([node, context]); + return `node-${node}-created-context`; + }; + const visitExit = (node: NodeId, context: mixed | null) => { + contexts.push(['exit', node, context]); + return `node-exit-${node}-created-context`; + }; + const getChildren = (node: NodeId) => + graph.getNodeIdsConnectedFrom(node); + const result = dfsImpl(graph, { + visit: { + enter: visit, + exit: visitExit, + }, + startNodeId: 0, + getChildren, + }); + + assert.deepEqual(contexts, [ + [0, undefined], + [1, 'node-0-created-context'], + [2, 'node-1-created-context'], + ['exit', 2, 'node-2-created-context'], + [3, 'node-1-created-context'], + ['exit', 3, 'node-3-created-context'], + ['exit', 1, 'node-1-created-context'], + ['exit', 0, 'node-0-created-context'], + ]); + assert.equal(result, undefined); + }); + }); + } + + testSuite('dfs', (graph, params) => graph.dfs(params)); + + testSuite('dfsNew', (graph, params) => graph.dfsNew(params)); + }); }); diff --git a/packages/core/integration-tests/test/cache.js b/packages/core/integration-tests/test/cache.js index f6a8bf07180..8f207b28bed 100644 --- a/packages/core/integration-tests/test/cache.js +++ b/packages/core/integration-tests/test/cache.js @@ -1319,6 +1319,7 @@ describe('cache', function () { featureFlags: { exampleFeature: false, configKeyInvalidation: true, + dfsFasterRefactor: false, }, async setup() { let pkgFile = path.join(inputDir, 'package.json'); @@ -1379,6 +1380,7 @@ describe('cache', function () { featureFlags: { exampleFeature: false, configKeyInvalidation: true, + dfsFasterRefactor: false, }, async setup() { let pkgFile = path.join(inputDir, 'package.json'); @@ -1439,6 +1441,7 @@ describe('cache', function () { featureFlags: { exampleFeature: false, configKeyInvalidation: true, + dfsFasterRefactor: false, }, async setup() { let pkgFile = path.join(inputDir, 'package.json');