-
Notifications
You must be signed in to change notification settings - Fork 4
feat: enhance file download and caching mechanisms #404
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,25 @@ | ||
| /** | ||
| * 4MB blocks — matches the chunk size used by the legacy downloader, proven to work well | ||
| * for this codebase. Each block is downloaded in full on first access regardless of how | ||
| * small the FUSE read is, so subsequent reads within the same block are served from disk. | ||
| * 4MB default blocks — lower latency on slow links while preserving cache locality. | ||
| * Each block is downloaded in full on first access regardless of how small the FUSE read is, | ||
| * so subsequent reads within the same block are served from disk. | ||
| */ | ||
| export const BLOCK_SIZE = 4 * 1024 * 1024; | ||
| const DEFAULT_BLOCK_SIZE_MB = 4; | ||
| const ALLOWED_BLOCK_SIZE_MB = new Set([1, 2, 4, 8]); | ||
|
|
||
| function getConfiguredBlockSizeInMb() { | ||
| const configuredValue = process.env.INTERNXT_DRIVE_DOWNLOAD_BLOCK_SIZE_MB; | ||
|
|
||
| if (!configuredValue) { | ||
| return DEFAULT_BLOCK_SIZE_MB; | ||
| } | ||
|
|
||
| const parsedValue = Number.parseInt(configuredValue, 10); | ||
| if (!ALLOWED_BLOCK_SIZE_MB.has(parsedValue)) { | ||
| return DEFAULT_BLOCK_SIZE_MB; | ||
| } | ||
|
|
||
| return parsedValue; | ||
| } | ||
|
|
||
| export const BLOCK_SIZE = getConfiguredBlockSizeInMb() * 1024 * 1024; | ||
| export const BITS_PER_BYTE = 8; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,6 +36,7 @@ export async function downloadAndCacheBlock({ | |
| blockLength, | ||
| }: Props): Promise<Result<void, Error>> { | ||
| if (isAborted(state)) return { data: undefined }; | ||
| if (blockLength <= 0 || blockStart >= virtualFile.size) return { data: undefined }; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this something that can happen? meaning: Fuse can ask for a block that is negative?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not just to validate negative ranges; it's also to prevent readings in the 0 range—which fuse does sometimes—and to ensure that invalid ranges aren't attempted, which is rare but does happen. |
||
|
|
||
| try { | ||
| const download = await downloadBlockWithRetry({ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,8 +10,18 @@ export function expandToBlockBoundaries({ range, fileSize }: { range: ReadRange; | |
| blockStart: number; | ||
| blockLength: number; | ||
| } { | ||
| const blockStart = Math.floor(range.position / BLOCK_SIZE) * BLOCK_SIZE; | ||
| const end = range.position + range.length; | ||
| const blockEnd = Math.min(Math.ceil(end / BLOCK_SIZE) * BLOCK_SIZE, fileSize); | ||
| if (fileSize <= 0 || range.length <= 0 || range.position >= fileSize) { | ||
| return { blockStart: Math.max(0, Math.min(range.position, fileSize)), blockLength: 0 }; | ||
| } | ||
|
|
||
| const clampedStart = Math.max(0, range.position); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why? i assume that range will always be 0 or more
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We might assume that's the case, but there are very rare instances where it might not be; this change is a simple safeguard that definitively prevents that from happening. |
||
| const clampedEndExclusive = Math.min(clampedStart + range.length, fileSize); | ||
|
|
||
| if (clampedEndExclusive <= clampedStart) { | ||
| return { blockStart: clampedStart, blockLength: 0 }; | ||
| } | ||
|
|
||
| const blockStart = Math.floor(clampedStart / BLOCK_SIZE) * BLOCK_SIZE; | ||
| const blockEnd = Math.min(Math.ceil(clampedEndExclusive / BLOCK_SIZE) * BLOCK_SIZE, fileSize); | ||
| return { blockStart, blockLength: blockEnd - blockStart }; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,12 +1,7 @@ | ||
| import { logger } from '@internxt/drive-desktop-core/build/backend'; | ||
| import { type TemporalFile } from '../../../../context/storage/TemporalFiles/domain/TemporalFile'; | ||
| import { type File } from '../../../../context/virtual-drive/files/domain/File'; | ||
| import { | ||
| type FuseError, | ||
| FuseIOError, | ||
| FuseNoSuchFileOrDirectoryError, | ||
| } from '../../../../apps/drive/fuse/callbacks/FuseErrors'; | ||
| import { downloadFileRange } from '../../../../infra/environment/download-file/download-file'; | ||
| import { type FuseError, FuseNoSuchFileOrDirectoryError } from '../../../../apps/drive/fuse/callbacks/FuseErrors'; | ||
| import { type Result } from '../../../../context/shared/domain/Result'; | ||
| import { readChunkFromDisk } from './read-chunk-from-disk'; | ||
| import nodePath from 'node:path'; | ||
|
|
@@ -15,6 +10,35 @@ import { EMPTY } from './constants'; | |
| import { readOrHydrate } from './read-or-hydrate'; | ||
| import { type HandleReadDeps, type ReadRange } from './types'; | ||
| import { isThumbnailProcess } from './thumbnail-processes'; | ||
|
|
||
| const PREFETCH_DEFAULT_BLOCKS_AHEAD = 5; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. shouldn this be on constants?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right, I've moved it. |
||
| const PREFETCH_MAX_BLOCKS_AHEAD = 8; | ||
|
|
||
| function getPrefetchBlocksAhead({ configuredValue }: { configuredValue: string | undefined }) { | ||
| if (!configuredValue) { | ||
| return PREFETCH_DEFAULT_BLOCKS_AHEAD; | ||
| } | ||
|
|
||
| const parsed = Number.parseInt(configuredValue, 10); | ||
| if (Number.isNaN(parsed)) { | ||
| return PREFETCH_DEFAULT_BLOCKS_AHEAD; | ||
| } | ||
|
|
||
| return Math.max(0, Math.min(parsed, PREFETCH_MAX_BLOCKS_AHEAD)); | ||
| } | ||
|
|
||
| function getThumbnailPrefetchBlocksAhead() { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why, for a thumbnail, we need to do a prefetch? would not this slow the initial load of the file explorer?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've been running tests focused on thumbnails, and you're right—the improvement is very small, and it slows down the network for other processes. |
||
| return getPrefetchBlocksAhead({ | ||
| configuredValue: process.env.INTERNXT_DRIVE_THUMBNAIL_PREFETCH_BLOCKS_AHEAD, | ||
| }); | ||
| } | ||
|
|
||
| function getReadPrefetchBlocksAhead() { | ||
| return getPrefetchBlocksAhead({ | ||
| configuredValue: process.env.INTERNXT_DRIVE_READ_PREFETCH_BLOCKS_AHEAD, | ||
| }); | ||
| } | ||
|
Comment on lines
+30
to
+40
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As I mentioned earlier, the idea is to be able to easily change the values. |
||
|
|
||
| export type HandleReadCallbackProps = HandleReadDeps & { | ||
| findVirtualFile: (path: string) => Promise<File | undefined>; | ||
| findTemporalFile: (path: string) => Promise<TemporalFile | undefined>; | ||
|
|
@@ -50,11 +74,25 @@ export async function handleReadCallback({ | |
|
|
||
| if (isThumbnailProcess(processName)) { | ||
| logger.debug({ | ||
| msg: '[ReadCallback] thumbnail process, downloading exact range', | ||
| msg: '[ReadCallback] thumbnail process, reading through cache hydration', | ||
| process: processName, | ||
| file: virtualFile.nameWithExtension, | ||
| }); | ||
| return readExactRangeForThumbnail({ bucketId, mnemonic, network, virtualFile, range }); | ||
|
|
||
| const filePath = nodePath.join(PATHS.DOWNLOADED, virtualFile.contentsId); | ||
| return readOrHydrate({ | ||
| bucketId, | ||
| mnemonic, | ||
| network, | ||
| // Thumbnail reads should not spam progress updates in UI. | ||
| onDownloadProgress: () => undefined, | ||
| // Thumbnail reads should not register files as offline available. | ||
| saveToRepository: async () => undefined, | ||
| virtualFile, | ||
| filePath, | ||
| range, | ||
| prefetchBlocksAhead: getThumbnailPrefetchBlocksAhead(), | ||
| }); | ||
| } | ||
|
|
||
| const filePath = nodePath.join(PATHS.DOWNLOADED, virtualFile.contentsId); | ||
|
|
@@ -68,6 +106,7 @@ export async function handleReadCallback({ | |
| virtualFile, | ||
| filePath, | ||
| range, | ||
| prefetchBlocksAhead: getReadPrefetchBlocksAhead(), | ||
| }); | ||
| } | ||
|
|
||
|
|
@@ -87,27 +126,3 @@ async function readFromTemporalFile( | |
| const chunk = await readChunkFromDisk(temporalFile.contentFilePath, length, position); | ||
| return { data: chunk ?? EMPTY }; | ||
| } | ||
|
|
||
| type ThumbnailRangeProps = Pick<HandleReadCallbackProps, 'bucketId' | 'mnemonic' | 'network' | 'range'> & { | ||
| virtualFile: File; | ||
| }; | ||
|
|
||
| async function readExactRangeForThumbnail({ | ||
| bucketId, | ||
| mnemonic, | ||
| network, | ||
| virtualFile, | ||
| range, | ||
| }: ThumbnailRangeProps): Promise<Result<Buffer, FuseError>> { | ||
| const { signal } = new AbortController(); | ||
| const result = await downloadFileRange({ | ||
| fileId: virtualFile.contentsId, | ||
| bucketId, | ||
| mnemonic, | ||
| network, | ||
| range, | ||
| signal, | ||
| }); | ||
| if (result.error) return { error: new FuseIOError(result.error.message) }; | ||
| return { data: result.data }; | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this needs to be a enviroment variable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My idea is that these values should be easy to change. Right now, the optimal settings are 4 MB per download and 3 prefetch blocks. These values can be increased if changes are made to the backend to reduce the latency of download requests, which would greatly improve performance.
Looking back, we can adjust these values to achieve better performance if the conditions are right; that’s why I set them as environment variables so they can be easily modified.