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

DFS refactor work #9648

Merged
merged 11 commits into from
Apr 22, 2024
Merged
4 changes: 4 additions & 0 deletions packages/core/core/src/BundleGraph.js
Original file line number Diff line number Diff line change
Expand Up @@ -1386,6 +1386,10 @@ export default class BundleGraph {
});
}

/**
* TODO: Document why this works like this & why visitor order matters
* on these use-cases.
*/
traverseBundle<TContext>(
bundle: Bundle,
visit: GraphVisitor<AssetNode | DependencyNode, TContext>,
Expand Down
1 change: 1 addition & 0 deletions packages/core/core/test/test-utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ export const DEFAULT_OPTIONS: ParcelOptions = {
featureFlags: {
exampleFeature: false,
configKeyInvalidation: false,
dfsFasterRefactor: false,
},
};

Expand Down
1 change: 1 addition & 0 deletions packages/core/feature-flags/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down
4 changes: 4 additions & 0 deletions packages/core/feature-flags/src/types.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
|};
1 change: 1 addition & 0 deletions packages/core/graph/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
"node": ">= 16.0.0"
},
"dependencies": {
"@parcel/feature-flags": "2.12.0",
"nullthrows": "^1.1.1"
}
}
4 changes: 2 additions & 2 deletions packages/core/graph/src/AdjacencyList.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ export type AdjacencyListOptions<TEdgeType> = {|
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,
Expand Down Expand Up @@ -328,7 +328,7 @@ export default class AdjacencyList<TEdgeType: number = 1> {
*
* 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.
Expand Down
179 changes: 169 additions & 10 deletions packages/core/graph/src/Graph.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -28,6 +29,51 @@ export type SerializedGraph<TNode, TEdgeType: number = 1> = {|
export type AllEdgeTypes = -1;
export const ALL_EDGE_TYPES: AllEdgeTypes = -1;

type DFSCommandVisit<TContext> = {|
nodeId: NodeId,
context: TContext | null,
|};

type DFSCommandExit<TContext> = {|
nodeId: NodeId,
exit: GraphTraversalCallback<NodeId, TContext>,
context: TContext | null,
|};

/**
* Internal type used for queue iterative DFS implementation.
*/
type DFSCommand<TContext> =
| DFSCommandVisit<TContext>
| DFSCommandExit<TContext>;

/**
* Options for DFS traversal.
*/
export type DFSParams<TContext> = {|
visit: GraphVisitor<NodeId, TContext>,
/**
* 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<NodeId>,
startNodeId?: ?NodeId,
|};

export default class Graph<TNode, TEdgeType: number = 1> {
nodes: Array<TNode | null>;
adjacencyList: AdjacencyList<TEdgeType>;
Expand Down Expand Up @@ -289,11 +335,17 @@ export default class Graph<TNode, TEdgeType: number = 1> {
) {
return this.dfsFast(enter, startNodeId);
yamadapc marked this conversation as resolved.
Show resolved Hide resolved
} 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),
});
}
}

Expand Down Expand Up @@ -449,15 +501,122 @@ export default class Graph<TNode, TEdgeType: number = 1> {
return;
}

/**
* Iterative implementation of DFS that supports all use-cases.
*
* This replaces `dfs` and will replace `dfsFast`.
*/
dfsNew<TContext>({
visit,
startNodeId,
getChildren,
}: DFSParams<TContext>): ?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<TContext>[] = [
{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') {
Copy link
Contributor

Choose a reason for hiding this comment

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

/nit != null checks are usually preferred in the codebase from what I can tell

// $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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We assign enter to a const up on line 545 - any reason we don't do the same for exit? I'm guessing a typeof isn't a huge overhead anyway..

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<TContext>({
visit,
startNodeId,
getChildren,
}: {|
visit: GraphVisitor<NodeId, TContext>,
getChildren(nodeId: NodeId): Array<NodeId>,
startNodeId?: ?NodeId,
|}): ?TContext {
}: DFSParams<TContext>): ?TContext {
let traversalStartNode = nullthrows(
startNodeId ?? this.rootNodeId,
'A start node is required to traverse',
Expand Down
Loading
Loading