Investigate: image upload consumes a lot of memory

[david-crespo]

Below is just suggestive — I didn't look far enough into it to be sure there's an issue, but it doesn't look very good. I also wasn't able to get the issue in Chrome, so maybe v8 is smarter about this than SpiderMonkey.

---

Inspired by https://schiener.io/2024-05-29/react-query-leaks, I was curious whether I could get the console to hold onto a bunch of memory. I didn't see an issue with normal browsing, which makes sense because it is mostly about closures containing large request bodies, and our request bodies are tiny.

However, image upload sends the entire contents of an image in chunks, so that seemed like a good candidate. Sure enough, taking snapshots showed memory increasing and not getting GCed, even when I cancel the upload and navigate around for a bit. All the memory is in "strings", which makes sense because we are reading each chunk as a base64ed string.

console/app/forms/image-upload.tsx

Lines 390 to 415 in 13ce748

	const postChunk = async (i: number) => {
	const offset = i * CHUNK_SIZE_BYTES
	const end = Math.min(offset + CHUNK_SIZE_BYTES, imageFile.size)
	const base64EncodedData = await readBlobAsBase64(imageFile.slice(offset, end))
	// Disk space is all zeros by default, so we can skip any chunks that are
	// all zeros. It turns out this happens a lot.
	if (!isAllZeros(base64EncodedData)) {
	await uploadChunk
	.mutateAsync({
	path,
	body: { offset, base64EncodedData },
	// use both the abort signal for the whole upload and a per-request timeout
	signal: anySignal([
	AbortSignal.timeout(30000),
	abortController.current?.signal,
	]),
	})
	.catch(() => {
	// this needs to throw a regular Error or pRetry gets mad
	throw Error(`Chunk ${i} (offset ${offset}) failed`)
	})
	}
	chunksProcessed++
	setUploadProgress(Math.round((100 * chunksProcessed) / nChunks))
	}

Using Firefox nightly, I was able to get a profile showing a rather huge (I would bet overstated) amount of memory being used during image upload.

2024-06-07 11.24 firefox-nightly-profile.json.gz

[david-crespo]

I did some experimenting here and I think it does actually consume just as much memory in Chrome.

The only change I was able to make that made it stop holding onto so much memory was commenting out the actual mutateAsync call, leaving everything else in place, including the part where we read the chunk from the file.

console/app/forms/image-upload.tsx

Lines 390 to 415 in 226d421

	const postChunk = async (i: number) => {
	const offset = i * CHUNK_SIZE_BYTES
	const end = Math.min(offset + CHUNK_SIZE_BYTES, imageFile.size)
	const base64EncodedData = await readBlobAsBase64(imageFile.slice(offset, end))
	// Disk space is all zeros by default, so we can skip any chunks that are
	// all zeros. It turns out this happens a lot.
	if (!isAllZeros(base64EncodedData)) {
	await uploadChunk
	.mutateAsync({
	path,
	body: { offset, base64EncodedData },
	// use both the abort signal for the whole upload and a per-request timeout
	signal: anySignal([
	AbortSignal.timeout(30000),
	abortController.current?.signal,
	]),
	})
	.catch(() => {
	// this needs to throw a regular Error or pRetry gets mad
	throw Error(`Chunk ${i} (offset ${offset}) failed`)
	})
	}
	chunksProcessed++
	setUploadProgress(Math.round((100 * chunksProcessed) / nChunks))
	}

That is a very strong clue, because it suggests the part where we read the chunk contents is fine. I learned RQ has a MutationCache, though I don't really understand why it exists. I guess it must be the place where the response is stored for you to look at in the mutation result. But it stores all the calls, not just the latest one. It makes me think that maybe we want to use a separate dedicated QueryClient (the thing that wraps the mutation cache) for the upload mutation that doesn't cache the results, or caches them for a shorter time. We don't have to get too elaborate for that: useMutation takes a third param that is a custom QueryClient, so all we'd have to do is add the optional third arg here and pass it through.

console/app/api/hooks.ts

Lines 226 to 240 in 226d421

	export const getUseApiMutation =
	<A extends ApiClient>(api: A) =>
	<M extends string & keyof A>(
	method: M,
	options?: Omit<
	UseMutationOptions<Result<A[M]>, ApiError, Params<A[M]> & { signal?: AbortSignal }>,
	'mutationFn'
	>
	) =>
	useMutation({
	mutationFn: ({ signal, ...params }) =>
	api[method](params, { signal }).then(handleResult(method)),
	// no catch, let unexpected errors bubble up
	...options,
	})

One strike against the mutation cache theory is that I tried running queryClient.getMutationCache().clear() after every chunk and it didn't seem to help the memory issue.