feat: enhance file download and caching mechanisms#404
Conversation
|
| const ALLOWED_BLOCK_SIZE_MB = new Set([1, 2, 4, 8]); | ||
|
|
||
| function getConfiguredBlockSizeInMb() { | ||
| const configuredValue = process.env.INTERNXT_DRIVE_DOWNLOAD_BLOCK_SIZE_MB; |
There was a problem hiding this comment.
Why this needs to be a enviroment variable
There was a problem hiding this comment.
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.
| 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.
Is this something that can happen? meaning: Fuse can ask for a block that is negative?
There was a problem hiding this comment.
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.
| 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.
Why? i assume that range will always be 0 or more
There was a problem hiding this comment.
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.
| import { type HandleReadDeps, type ReadRange } from './types'; | ||
| import { isThumbnailProcess } from './thumbnail-processes'; | ||
|
|
||
| const PREFETCH_DEFAULT_BLOCKS_AHEAD = 5; |
| function getThumbnailPrefetchBlocksAhead() { | ||
| return getPrefetchBlocksAhead({ | ||
| configuredValue: process.env.INTERNXT_DRIVE_THUMBNAIL_PREFETCH_BLOCKS_AHEAD, | ||
| }); | ||
| } | ||
|
|
||
| function getReadPrefetchBlocksAhead() { | ||
| return getPrefetchBlocksAhead({ | ||
| configuredValue: process.env.INTERNXT_DRIVE_READ_PREFETCH_BLOCKS_AHEAD, | ||
| }); | ||
| } |
There was a problem hiding this comment.
As I mentioned earlier, the idea is to be able to easily change the values.
| return Math.max(0, Math.min(parsed, PREFETCH_MAX_BLOCKS_AHEAD)); | ||
| } | ||
|
|
||
| function getThumbnailPrefetchBlocksAhead() { |
There was a problem hiding this comment.
Why, for a thumbnail, we need to do a prefetch? would not this slow the initial load of the file explorer?
There was a problem hiding this comment.
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.
| } | ||
| } | ||
|
|
||
| function schedulePrefetchBlocksAhead({ |
There was a problem hiding this comment.
Shouldnt we test this separtedly since it has a lot of logic?
| }): Promise<Result<void, Error>> { | ||
| const { blockStart, blockLength } = expandToBlockBoundaries({ range, fileSize: virtualFile.size }); | ||
|
|
||
| if (blockLength <= 0 || blockStart >= virtualFile.size) { |
There was a problem hiding this comment.
Again, if this is a realistic scenario, why not prevent from calling this whole functionality?
| const downloads = missingBlocks.map((block) => { | ||
| const start = block * BLOCK_SIZE; | ||
| const end = Math.min(start + BLOCK_SIZE, virtualFile.size); | ||
| const boundedBlockLength = end - start; |
There was a problem hiding this comment.
Same, i feel like there has to be a way to prevent this whole "negative block request" instead of just adding everywhere the check, this just adds unnecessary complexity dont you think
| if ( | ||
| !shouldEmitProgress({ | ||
| now: Date.now(), | ||
| bytesDownloaded, | ||
| fileSize, | ||
| state: progressReporterState, | ||
| }) | ||
| ) { | ||
| return; | ||
| } | ||
|
|
| const PROGRESS_UPDATE_INTERVAL_MS = 250; | ||
|
|
||
| type ProgressReporterState = { | ||
| lastUpdateAt: number; | ||
| }; | ||
|
|
||
| function shouldEmitProgress({ | ||
| now, | ||
| bytesDownloaded, | ||
| fileSize, | ||
| state, | ||
| }: { | ||
| now: number; | ||
| bytesDownloaded: number; | ||
| fileSize: number; | ||
| state: ProgressReporterState; | ||
| }) { | ||
| const reachedEnd = bytesDownloaded >= fileSize; | ||
| const elapsedSinceLastUpdate = now - state.lastUpdateAt; | ||
|
|
||
| if (!reachedEnd && elapsedSinceLastUpdate < PROGRESS_UPDATE_INTERVAL_MS) { | ||
| return false; | ||
| } | ||
|
|
||
| state.lastUpdateAt = now; | ||
| return true; | ||
| } | ||
|
|
There was a problem hiding this comment.
IF this is necessary this should be in its own file, where we should test it separatedly
| let bytesRead = 0; | ||
| let buffer = Buffer.alloc(length); | ||
|
|
||
| response.data.on('data', (chunk: Uint8Array) => { | ||
| const source = Buffer.from(chunk); | ||
| const requiredLength = bytesRead + source.length; | ||
|
|
||
| if (requiredLength > buffer.length) { | ||
| const next = Buffer.alloc(Math.max(buffer.length * 2, requiredLength)); | ||
| buffer.copy(next, 0, 0, bytesRead); | ||
| buffer = next; | ||
| } | ||
|
|
||
| source.copy(buffer, bytesRead); | ||
| bytesRead += source.length; | ||
| }); |
2 participants
What is Changed / Added
Why