From 4659f5d6e31d24cc2294641bc4facba174e650a2 Mon Sep 17 00:00:00 2001 From: Ben Jervis Date: Thu, 16 May 2024 15:19:28 +1000 Subject: [PATCH 1/6] Make nodes per blob a method to allow overriding --- packages/core/core/src/RequestTracker.js | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/packages/core/core/src/RequestTracker.js b/packages/core/core/src/RequestTracker.js index e8fb95d735b..24b1f94386c 100644 --- a/packages/core/core/src/RequestTracker.js +++ b/packages/core/core/src/RequestTracker.js @@ -263,6 +263,10 @@ const keyFromEnvContentKey = (contentKey: ContentKey): string => const keyFromOptionContentKey = (contentKey: ContentKey): string => contentKey.slice('option:'.length); +// This constant is chosen by local profiling the time to serialise n nodes and tuning until an average time of ~50 ms per blob. +// The goal is to free up the event loop periodically to allow interruption by the user. +const NODES_PER_BLOB = 2 ** 14; + export class RequestGraph extends ContentGraph< RequestGraphNode, RequestGraphEdgeType, @@ -279,6 +283,7 @@ export class RequestGraph extends ContentGraph< invalidateOnBuildNodeIds: Set = new Set(); cachedRequestChunks: Set = new Set(); configKeyNodes: Map> = new Map(); + nodesPerBlob: number = NODES_PER_BLOB; // $FlowFixMe[prop-missing] static deserialize(opts: RequestGraphOpts): RequestGraph { @@ -1026,14 +1031,10 @@ export class RequestGraph extends ContentGraph< } removeCachedRequestChunkForNode(nodeId: number): void { - this.cachedRequestChunks.delete(Math.floor(nodeId / NODES_PER_BLOB)); + this.cachedRequestChunks.delete(Math.floor(nodeId / this.nodesPerBlob)); } } -// This constant is chosen by local profiling the time to serialise n nodes and tuning until an average time of ~50 ms per blob. -// The goal is to free up the event loop periodically to allow interruption by the user. -const NODES_PER_BLOB = 2 ** 14; - export default class RequestTracker { graph: RequestGraph; farm: WorkerFarm; @@ -1421,7 +1422,11 @@ export default class RequestTracker { } } - for (let i = 0; i * NODES_PER_BLOB < cacheableNodes.length; i += 1) { + for ( + let i = 0; + i * this.graph.nodesPerBlob < cacheableNodes.length; + i += 1 + ) { if (!this.graph.hasCachedRequestChunk(i)) { // We assume the request graph nodes are immutable and won't change queue @@ -1429,8 +1434,8 @@ export default class RequestTracker { serialiseAndSet( getRequestGraphNodeKey(i, cacheKey), cacheableNodes.slice( - i * NODES_PER_BLOB, - (i + 1) * NODES_PER_BLOB, + i * this.graph.nodesPerBlob, + (i + 1) * this.graph.nodesPerBlob, ), ).then(() => { // Succeeded in writing to disk, save that we have completed this chunk From 39f373982eb088078472f6047133db2d38f0efa5 Mon Sep 17 00:00:00 2001 From: Ben Jervis Date: Thu, 16 May 2024 15:19:50 +1000 Subject: [PATCH 2/6] Add a test for the stale cache issue --- .../core/core/test/RequestTracker.test.js | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/packages/core/core/test/RequestTracker.test.js b/packages/core/core/test/RequestTracker.test.js index 8315445b00f..d2a27979957 100644 --- a/packages/core/core/test/RequestTracker.test.js +++ b/packages/core/core/test/RequestTracker.test.js @@ -307,4 +307,26 @@ describe('RequestTracker', () => { assert.strictEqual(cachedResult, 'b'); assert.strictEqual(called, false); }); + + it.only('should use the request graph ID when writing nodes to cache', async () => { + let tracker = new RequestTracker({farm, options}); + + // Set the nodes per blob low so we can ensure multiple files without + // creating 17,000 nodes + tracker.graph.nodesPerBlob = 2; + + tracker.graph.addNode({type: 0, id: 'some-file-node-1'}); + tracker.graph.addNode({type: 0, id: 'some-file-node-2'}); + tracker.graph.addNode({type: 0, id: 'some-file-node-3'}); + + await tracker.writeToCache(); + + tracker = new RequestTracker({farm, options}); + assert.equal(tracker.graph.nodes.length, 0); + + await tracker.writeToCache(); + + tracker = await RequestTracker.init({farm, options}); + assert.equal(tracker.graph.nodes.length, 0); + }); }); From d0d38fa415a1b1e0c10ad0cde47c80255a01cefd Mon Sep 17 00:00:00 2001 From: Ben Jervis Date: Fri, 17 May 2024 10:35:50 +1000 Subject: [PATCH 3/6] Update tests to cover more cases --- .../core/core/test/RequestTracker.test.js | 28 +++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-) diff --git a/packages/core/core/test/RequestTracker.test.js b/packages/core/core/test/RequestTracker.test.js index d2a27979957..bd7ed45d641 100644 --- a/packages/core/core/test/RequestTracker.test.js +++ b/packages/core/core/test/RequestTracker.test.js @@ -308,7 +308,7 @@ describe('RequestTracker', () => { assert.strictEqual(called, false); }); - it.only('should use the request graph ID when writing nodes to cache', async () => { + it('should ignore stale node chunks from cache', async () => { let tracker = new RequestTracker({farm, options}); // Set the nodes per blob low so we can ensure multiple files without @@ -318,15 +318,39 @@ describe('RequestTracker', () => { tracker.graph.addNode({type: 0, id: 'some-file-node-1'}); tracker.graph.addNode({type: 0, id: 'some-file-node-2'}); tracker.graph.addNode({type: 0, id: 'some-file-node-3'}); + tracker.graph.addNode({type: 0, id: 'some-file-node-4'}); + tracker.graph.addNode({type: 0, id: 'some-file-node-5'}); await tracker.writeToCache(); + // Create a new request tracker that shouldn't look at the old cache files tracker = new RequestTracker({farm, options}); assert.equal(tracker.graph.nodes.length, 0); + tracker.graph.addNode({type: 0, id: 'some-file-node-1'}); await tracker.writeToCache(); + // Init a request tracker that should only read the relevant cache files tracker = await RequestTracker.init({farm, options}); - assert.equal(tracker.graph.nodes.length, 0); + assert.equal(tracker.graph.nodes.length, 1); + }); + + it('should init with multiple node chunks', async () => { + let tracker = new RequestTracker({farm, options}); + + // Set the nodes per blob low so we can ensure multiple files without + // creating 17,000 nodes + tracker.graph.nodesPerBlob = 2; + + tracker.graph.addNode({type: 0, id: 'some-file-node-1'}); + tracker.graph.addNode({type: 0, id: 'some-file-node-2'}); + tracker.graph.addNode({type: 0, id: 'some-file-node-3'}); + tracker.graph.addNode({type: 0, id: 'some-file-node-4'}); + tracker.graph.addNode({type: 0, id: 'some-file-node-5'}); + + await tracker.writeToCache(); + + tracker = await RequestTracker.init({farm, options}); + assert.equal(tracker.graph.nodes.length, 5); }); }); From 3ff49bcc9cd4ebe86d0d5e94cc86d2d93013e094 Mon Sep 17 00:00:00 2001 From: Ben Jervis Date: Fri, 17 May 2024 10:36:09 +1000 Subject: [PATCH 4/6] Write the number of nodes in each blob to the cache --- packages/core/core/src/RequestTracker.js | 42 ++++++++++++++++-------- 1 file changed, 29 insertions(+), 13 deletions(-) diff --git a/packages/core/core/src/RequestTracker.js b/packages/core/core/src/RequestTracker.js index 24b1f94386c..9050c05956a 100644 --- a/packages/core/core/src/RequestTracker.js +++ b/packages/core/core/src/RequestTracker.js @@ -1422,21 +1422,31 @@ export default class RequestTracker { } } + let nodeCountsPerBlob = []; + for ( let i = 0; i * this.graph.nodesPerBlob < cacheableNodes.length; i += 1 ) { + let nodesStartIndex = i * this.graph.nodesPerBlob; + let nodesEndIndex = (i + 1) * this.graph.nodesPerBlob; + + if (nodesEndIndex > cacheableNodes.length) { + nodesEndIndex = cacheableNodes.length; + } + + nodeCountsPerBlob.push(nodesEndIndex - nodesStartIndex); + if (!this.graph.hasCachedRequestChunk(i)) { // We assume the request graph nodes are immutable and won't change + let nodesToCache = cacheableNodes.slice(nodesStartIndex, nodesEndIndex); + queue .add(() => serialiseAndSet( getRequestGraphNodeKey(i, cacheKey), - cacheableNodes.slice( - i * this.graph.nodesPerBlob, - (i + 1) * this.graph.nodesPerBlob, - ), + nodesToCache, ).then(() => { // Succeeded in writing to disk, save that we have completed this chunk this.graph.setCachedRequestChunk(i); @@ -1454,6 +1464,7 @@ export default class RequestTracker { // Set the request graph after the queue is flushed to avoid writing an invalid state await serialiseAndSet(requestGraphKey, { ...serialisedGraph, + nodeCountsPerBlob, nodes: undefined, }); @@ -1492,7 +1503,7 @@ export function getWatcherOptions({ watchBackend, }: ParcelOptions): WatcherOptions { const vcsDirs = ['.git', '.hg']; - const uniqueDirs = [...new Set([...watchIgnore, ...vcsDirs, cacheDir])]; + const uniqueDirs = [...new Set([...watchIgnore, cacheDir, ...vcsDirs])]; const ignore = uniqueDirs.map(dir => path.resolve(watchDir, dir)); return {ignore, backend: watchBackend}; @@ -1522,19 +1533,24 @@ export async function readAndDeserializeRequestGraph( return deserialize(buffer); }; - let i = 0; - let nodePromises = []; - while (await cache.hasLargeBlob(getRequestGraphNodeKey(i, cacheKey))) { - nodePromises.push(getAndDeserialize(getRequestGraphNodeKey(i, cacheKey))); - i += 1; - } - let serializedRequestGraph = await getAndDeserialize(requestGraphKey); + let nodePromises = serializedRequestGraph.nodeCountsPerBlob.map( + async (nodesCount, i) => { + let nodes = await getAndDeserialize(getRequestGraphNodeKey(i, cacheKey)); + invariant.equal( + nodes.length, + nodesCount, + 'RequestTracker node chunk: invalid node count', + ); + return nodes; + }, + ); + return { requestGraph: RequestGraph.deserialize({ ...serializedRequestGraph, - nodes: (await Promise.all(nodePromises)).flatMap(nodeChunk => nodeChunk), + nodes: (await Promise.all(nodePromises)).flat(), }), // This is used inside parcel query for `.inspectCache` bufferLength, From 92a3660c1d29194e775f05c8a89fe40e1fb4087f Mon Sep 17 00:00:00 2001 From: Ben Jervis Date: Fri, 17 May 2024 10:43:04 +1000 Subject: [PATCH 5/6] Undo erroneous variable move --- packages/core/core/src/RequestTracker.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/core/src/RequestTracker.js b/packages/core/core/src/RequestTracker.js index 9050c05956a..d287977f48a 100644 --- a/packages/core/core/src/RequestTracker.js +++ b/packages/core/core/src/RequestTracker.js @@ -1503,7 +1503,7 @@ export function getWatcherOptions({ watchBackend, }: ParcelOptions): WatcherOptions { const vcsDirs = ['.git', '.hg']; - const uniqueDirs = [...new Set([...watchIgnore, cacheDir, ...vcsDirs])]; + const uniqueDirs = [...new Set([...watchIgnore, ...vcsDirs, cacheDir])]; const ignore = uniqueDirs.map(dir => path.resolve(watchDir, dir)); return {ignore, backend: watchBackend}; From 2037d20c79f35c14c329ad3087d58e3ba3d1d8b9 Mon Sep 17 00:00:00 2001 From: Ben Jervis Date: Fri, 17 May 2024 10:47:08 +1000 Subject: [PATCH 6/6] Be extra mathsy --- packages/core/core/src/RequestTracker.js | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/packages/core/core/src/RequestTracker.js b/packages/core/core/src/RequestTracker.js index d287977f48a..d9b4e560f95 100644 --- a/packages/core/core/src/RequestTracker.js +++ b/packages/core/core/src/RequestTracker.js @@ -1430,11 +1430,10 @@ export default class RequestTracker { i += 1 ) { let nodesStartIndex = i * this.graph.nodesPerBlob; - let nodesEndIndex = (i + 1) * this.graph.nodesPerBlob; - - if (nodesEndIndex > cacheableNodes.length) { - nodesEndIndex = cacheableNodes.length; - } + let nodesEndIndex = Math.min( + (i + 1) * this.graph.nodesPerBlob, + cacheableNodes.length, + ); nodeCountsPerBlob.push(nodesEndIndex - nodesStartIndex);