-
Notifications
You must be signed in to change notification settings - Fork 5
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
1f8371d
481a78d
3d88862
cfbb7eb
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,46 @@ | ||
| /** | ||
| * 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]); | ||
| const PREFETCH_DEFAULT_BLOCKS_AHEAD = 5; | ||
| const PREFETCH_MAX_BLOCKS_AHEAD = 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; | ||
| } | ||
|
|
||
| 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)); | ||
| } | ||
|
|
||
| export function getReadPrefetchBlocksAhead() { | ||
| return getPrefetchBlocksAhead({ | ||
| configuredValue: process.env.INTERNXT_DRIVE_READ_PREFETCH_BLOCKS_AHEAD, | ||
| }); | ||
| } | ||
|
|
||
| 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 }; | ||
| } | ||
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.