Break up request graph cache serialisation and run after build completion #9384

JakeLane · 2023-11-15T07:03:51Z

↪️ Pull Request

Currently in Jira Frontend, we have a major problem with developers becoming confused when shutting down parcel, as it looks like the process has crashed. This is because the request tracker is serialised and written to cache, when the parcel cli is shutdown.

To avoid this, we can utilise the idle time in the watch process by serialising after the build has completed. It's also important to break up the serialisation phase, as we need to be able to interrupt at any time for a new build. I picked a sane constant value to split up the nodes in groups that took around ~10ms to process on my machine, which should free up the event loop well enough for most developers.

🚨 Test instructions

Start a parcel build in watch mode
Wait for build to complete
Observe cache serialising to disk
Close parcel (observe near-instant shutdown)

JakeLane · 2023-11-15T07:05:11Z

packages/core/core/src/Parcel.js

@@ -361,6 +358,7 @@ export default class Parcel {
        createValidationRequest({optionsRef: this.#optionsRef, assetRequests}),
        {force: assetRequests.length > 0},
      );
+      await this.#requestTracker.writeToCache();


This is after the build has reported as complete. Open to suggestions for a more suitable location.

JakeLane · 2023-11-15T07:05:45Z

packages/core/core/src/RequestTracker.js

-    let requestGraphKey = hashString(`${cacheKey}:requestGraph`);
-    let snapshotKey = hashString(`${cacheKey}:snapshot`);
+    const cacheKey = getCacheKey(this.options);
+    const requestGraphKey = `requestGraph:${hashString(cacheKey)}`;


I changed these as it's easier to debug what's happening if the cache folder has useful names

…tion

devongovett · 2023-11-16T04:44:30Z

Won't this end up spending more time/cpu serializing in general because it runs after every build instead of only when shutting down Parcel? Sure it starts earlier when you actually are shutting down but that's overall a pretty rare event vs doing a regular rebuild, in which case there's no need to save to disk.

JakeLane · 2023-11-16T06:59:50Z

Won't this end up spending more time/cpu serializing in general because it runs after every build instead of only when shutting down Parcel? Sure it starts earlier when you actually are shutting down but that's overall a pretty rare event vs doing a regular rebuild, in which case there's no need to save to disk.

Yeah, it's definitely a compromise as we'll need to use CPU more often, but it shouldn't impact active waiting time for devs by slowing down the shutdown of the process. The parcel build should be usable to the developer while this serialisation is happening (requests will be served) and it's interruptible, so new changes to source should cancel these writes.

I'm realising now my implementation isn't ideal here, as it doesn't get interrupted in the way I expected. I'll push up an update tomorrow which should make more sense.

devongovett · 2023-11-16T07:53:53Z

Even better would be to only serialize the parts of the graph that changed now that you have chunking...

JakeLane · 2023-11-17T00:53:09Z

Even better would be to only serialize the parts of the graph that changed now that you have chunking...

I wasn't able to come up with a way to know if it's safe to skip serialisation of a chunk efficiently :/

Ideally we need to know if a node has changed since last build and since it's possible for a node to be deleted, we'd have to have some sort of tracking for changed nodes.

…te-request-graph-to-disk-background

marcins

Just left some general feedback / questions for now.

marcins · 2023-12-12T05:21:04Z

packages/core/cache/src/FSCache.js

+      }
+    }
+
+    // If there's already a file following this chunk, it's old and should be removed


Does it matter if it had extra chunks after that, or does that just become "garbage"?

i.e. if you had chunks 0, 1, 2, 3 (unlikely given the sizes - but still..), and next time have 0,1, you'll delete 2 but leave 3 dangling?

Yeah I left it dangling as the intention was purely to cut the edge case of joining two different chunks together. In retrospect, it's not complex to just delete everything so I'll add that

marcins · 2023-12-12T05:22:54Z

packages/core/cache/src/FSCache.js

+    }
+
+    // If there's already a file following this chunk, it's old and should be removed
+    if (await this.fs.exists(this.#getFilePath(key, chunks))) {


Note that async existence checking is long deprecated in Node, and can lead to race conditions: https://nodejs.org/api/fs.html#fsexistspath-callback (i.e. the recommended approach would be to just try and unlink and ignore failure)

marcins · 2023-12-12T05:23:47Z

packages/core/cache/src/LMDBCache.js

@@ -111,27 +112,47 @@ export class LMDBCache implements Cache {
    return Buffer.concat(await Promise.all(buffers));
  }

-  async setLargeBlob(key: string, contents: Buffer | string): Promise<void> {
+  async setLargeBlob(


Is there any elegant way to share the implementation between the two Cache types as (AFAICT) they're identical?

Instantiate a FSCache instance inside lmdb cache and forward calls of setLargeBlob to that?

That sounds good to me, will clean this up

marcins · 2023-12-12T05:25:14Z

packages/core/core/src/RequestTracker.js

@@ -846,6 +846,8 @@ export class RequestGraph extends ContentGraph<
  }
 }

+const NODES_PER_BLOB = 2 ** 14;


How did we arrive at this number?

I profiled locally on my machine on how long it took to serialise n nodes. I tuned n with binary search until I reached approximately ~50 ms serialisation time per blob. The goal is to free up the event loop for a reasonable amount of time for user perception.

I'll document this on the constant so it can be tuned in the future if required.

marcins · 2023-12-12T05:36:11Z

packages/core/core/src/RequestTracker.js

-      total,
-      size: this.graph.nodes.length,
-    });
-    report({type: 'cache', phase: 'end', total, size: this.graph.nodes.length});

    await Promise.all(promises);


How many concurrent promises will be typical here for a Jira cache? Promise.all with a large set, especially when writing files or doing other async IO stuff, can have sub-optimal performance. If the concurrency is large enough, you might have better results using async/queue with a concurrency limit set.

(We also have PromiseQueue in the utils for exactly this use case you haven't seen it in the codebase yet.)

(We also have PromiseQueue in the utils for exactly this use case you haven't seen it in the codebase yet.)

Ah, yeah, it's even touched in this PR 😅

I was thinking of Jira where I have used async/queue in the past for this..

mattcompiles · 2024-02-06T03:37:32Z

packages/core/core/src/RequestTracker.js

-      hashString(`${cacheKey}:requestGraph`) + '-RequestGraph';
-    let snapshotKey = hashString(`${cacheKey}:snapshot`);
+    let requestGraphKey = `requestGraph-${hashString(cacheKey)}`;
+    let snapshotKey = `snapshot-${hashString(cacheKey)}`;


Not a biggy, but no point hashing the cache key twice.

mattcompiles · 2024-02-06T03:43:04Z

packages/core/core/src/RequestTracker.js

+      queue
+        .add(() =>
+          serialiseAndSet(
+            `requestGraph-nodes-${i}-${hashString(cacheKey)}`,


Same here, we could re-use the hashed cache key from earlier.

mattcompiles · 2024-02-06T03:59:09Z

packages/core/core/src/RequestTracker.js

+      )
+    ) {
+      nodePromises.push(
+        getAndDeserialize(`requestGraph-nodes-${i}-${hashedCacheKey}`),


We generate this string at least 3 different times. Could move it to a function so it's clear they're tied together?

mattcompiles · 2024-02-06T04:23:02Z

packages/core/core/src/RequestTracker.js

+          opts,
+        ),
+      )
+      .catch(() => {});


What's the point of this .catch?

This is to ensure we don't crash parcel if we interrupt a serialisation with the watcher (e.g. a new build is triggered)

Is this required even though we're not awaiting the result?

This is the "handler" to make sure it's not an "unhandled promise rejection"

I'll bring this back, I think that's the main reason it's required to have

mattcompiles · 2024-02-06T05:07:29Z

packages/core/core/src/RequestTracker.js

+    for (let i = 0; i * NODES_PER_BLOB < cacheableNodes.length; i += 1) {
+      if (
+        this.cachedRequestsLastChunk !== null &&
+        i < this.cachedRequestsLastChunk


The assumption here is that nodes never change or get re-ordered?

Yep added a comment here. This idea was from @devongovett in the sync

mattcompiles · 2024-02-06T05:10:59Z

packages/core/core/src/RequestTracker.js

+      if (
+        this.cachedRequestsLastChunk !== null &&
+        i < this.cachedRequestsLastChunk
+      ) {
        continue;


Maybe it'd be nicer, to calculate i with this expression rather than just calling continue at the start of the loop?

Refactored this a bit, let me know what you reckon

mattcompiles

Good job bud 👏. This was a tricky one to get through. It all makes logical sense to me now. Happy to merge assuming you've tested it extensively.

JakeLane · 2024-02-13T03:38:27Z

Discuss mutability of request nodes with @mattcompiles

mattcompiles · 2024-02-15T21:22:34Z

packages/core/core/src/RequestTracker.js

@@ -269,6 +273,7 @@ export class RequestGraph extends ContentGraph<
      optionNodeIds: this.optionNodeIds,
      unpredicatableNodeIds: this.unpredicatableNodeIds,
      invalidateOnBuildNodeIds: this.invalidateOnBuildNodeIds,
+      cachedRequestChunks: this.cachedRequestChunks,


Cool idea, this means we'll be able to skip work between different sessions of Parcel.

mattcompiles · 2024-02-15T21:27:55Z

packages/core/core/src/RequestTracker.js

@@ -345,6 +351,9 @@ export class RequestGraph extends ContentGraph<
    for (let parentNode of parentNodes) {
      this.invalidateNode(parentNode, reason);
    }
+
+    // If the node is invalidated, the cached request chunk on disk needs to be re-written
+    this.cachedRequestChunks.delete(Math.floor(nodeId / NODES_PER_BLOB));


I think we may need to do this from Request.completeRequest as well? I think it's possible that a node is invalidated but its request hasn't been re-run yet. Once it is re-run we'd want to update it's result in the cache.

mattcompiles

Approval for the new invalidation changes.

JakeLane requested review from devongovett, lettertwo, mattcompiles, gorakong, AGawrys and irismoini November 15, 2023 07:03

JakeLane commented Nov 15, 2023

View reviewed changes

Break up request graph cache serialisation and run after build comple…

c3fd8f2

…tion

JakeLane force-pushed the jlane2/write-request-graph-to-disk-background branch from f831368 to e5ffbea Compare November 16, 2023 04:33

JakeLane added 2 commits November 16, 2023 15:38

Fix test to abort before writing to cache

c06fa33

Write cache to disk when build fails

863b5bd

JakeLane force-pushed the jlane2/write-request-graph-to-disk-background branch from e5ffbea to 863b5bd Compare November 16, 2023 04:43

Move cache aborting to build queue

d93e422

JakeLane force-pushed the jlane2/write-request-graph-to-disk-background branch from 858efed to d93e422 Compare November 17, 2023 00:42

JakeLane added 3 commits November 17, 2023 16:07

Fix cache node shallow copy

63f0017

Merge branch 'v2' into jlane2/write-request-graph-to-disk-background

0cfdedb

Merge branch 'v2' of github.com:parcel-bundler/parcel into jlane2/wri…

d1e726d

…te-request-graph-to-disk-background

JakeLane force-pushed the jlane2/write-request-graph-to-disk-background branch from 1efe860 to d1e726d Compare December 8, 2023 03:41

JakeLane added 2 commits December 12, 2023 11:40

Resolve windows cache path issue

b82ab4c

Merge branch 'v2' into jlane2/write-request-graph-to-disk-background

07302ca

marcins reviewed Dec 12, 2023

View reviewed changes

JakeLane added 4 commits December 13, 2023 11:27

Simplify cache chunking by moving implementation to FSCache

80507da

Use promise queue to manage cache writes

730c274

Merge branch 'v2' into jlane2/write-request-graph-to-disk-background

e5d8565

Resolve unhandled errors with promise queue

51ffd03

JakeLane force-pushed the jlane2/write-request-graph-to-disk-background branch from 84ea07d to ed110a1 Compare January 25, 2024 03:56

github-actions bot deployed to Preview January 25, 2024 04:25 View deployment

JakeLane requested review from Nikola-3 and removed request for lettertwo, gorakong, AGawrys and irismoini January 30, 2024 05:42

Merge branch 'v2' into jlane2/write-request-graph-to-disk-background

48f1d25

github-actions bot deployed to Preview February 6, 2024 03:49 View deployment

mattcompiles reviewed Feb 6, 2024

View reviewed changes

Add getRequestGraphNodeKey function and refactor for loop

96c1abc

github-actions bot deployed to Preview February 8, 2024 01:42 View deployment

JakeLane added 3 commits February 8, 2024 15:11

Move to set to track cached requests to disk

c88cab5

Bring back catch on queue add

921c7ba

Update unit test for RequestTracker

6d339a6

github-actions bot deployed to Preview February 9, 2024 03:02 View deployment

mattcompiles approved these changes Feb 12, 2024

View reviewed changes

Update progress for cache write and handle node invalidation

3965abb

JakeLane force-pushed the jlane2/write-request-graph-to-disk-background branch from a5a153e to 3965abb Compare February 15, 2024 05:41

Update unit test to use new graph set

e0b3ba2

github-actions bot deployed to Preview February 15, 2024 06:37 View deployment

mattcompiles reviewed Feb 15, 2024

View reviewed changes

Invalidate written cache on disk on request completion

3d8b1e3

github-actions bot deployed to Preview February 15, 2024 22:34 View deployment

mattcompiles approved these changes Feb 15, 2024

View reviewed changes

Merge branch 'v2' into jlane2/write-request-graph-to-disk-background

0941434

github-actions bot deployed to Preview February 16, 2024 02:25 View deployment

JakeLane merged commit 0560499 into v2 Feb 16, 2024
14 of 16 checks passed

mischnic deleted the jlane2/write-request-graph-to-disk-background branch March 21, 2024 21:53

Break up request graph cache serialisation and run after build completion #9384

Break up request graph cache serialisation and run after build completion #9384

Conversation

JakeLane commented Nov 15, 2023

↪️ Pull Request

🚨 Test instructions

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devongovett commented Nov 16, 2023

JakeLane commented Nov 16, 2023

devongovett commented Nov 16, 2023

JakeLane commented Nov 17, 2023

marcins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattcompiles left a comment

Choose a reason for hiding this comment

JakeLane commented Feb 13, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattcompiles left a comment

Choose a reason for hiding this comment

JakeLane commented Feb 13, 2024 •

edited