googleapis / googleapis/google-cloud-swift

storage: eliminate lookahead buffer discard and redundant disk reads in ChecksummedSource

Open
#887 0 comments 0 reactions 0 assignees View on GitHub
api: storage
Dominant language
Swift
Stars
26
Forks
10
Avg merge
12h 56m
Merged PRs (30d)
211

Description

# storage: eliminate lookahead buffer discard and redundant disk reads in `ChecksummedSource`

## Problem Description

In `pkgs/swift-google-cloud-storage/Sources/GoogleCloudStorage/ChecksummedSource.swift`, chunked resumable uploads experience redundant I/O and memory churn because `readChunk` pre-fetches the subsequent chunk ahead of time, which is then discarded and re-read on every intermediate chunk upload.

### The Mechanism

1. **Pre-fetching in `readChunk(maxBytes:)`**:
In `ChecksummedSource.swift:102-117`:
```swift
mutating func readChunk(maxBytes: Int) async throws -> ChunkInfo? {
if !isInitialized {
nextChunk = try await source.read(maxBytes: maxBytes)
isInitialized = true
}
guard let currentChunk = nextChunk, !currentChunk.isEmpty else { return nil }
let currentChunkOffset = nextChunkOffset
nextChunkOffset += UInt64(currentChunk.count)

nextChunk = try await source.read(maxBytes: maxBytes) // <-- PRE-FETCHES NEXT CHUNK
let isLast = nextChunk == nil || nextChunk!.isEmpty
...
return ChunkInfo(data: currentChunk, isLast: isLast, checksum: checksumStr)
}
```
To determine whether `currentChunk` is the final chunk (`isLast`), `readChunk` immediately reads the *next* chunk into `nextChunk`.
When uploading a 128 MiB file with 32 MiB chunks:
- Chunk 0 (0..<32 MiB) is read into `currentChunk`.
- Chunk 1 (32..<64 MiB) is immediately read into `nextChunk`.

2. **Discard and Rewind on HTTP 308**:
After chunk 0 is transmitted via HTTP PUT, Google Cloud Storage acknowledges chunk 0 with `HTTP/1.1 308 Resume Incomplete` and `Range: bytes=0-33554431`.
In `StorageClient+Upload.swift:768`, the upload loop prepares for the next chunk by calling:
```swift
try await checksummedSource!.seek(to: committedBytes) // committedBytes = 32 MiB
```
Inside `ChecksummedSource.seek(to:)` (`ChecksummedSource.swift:146-154`):
```swift
mutating func seek(to offset: UInt64) async throws {
nextChunk = nil // <-- DISCARDS THE 32 MiB BUFFER JUST PRE-FETCHED
isInitialized = false
isFinished = false
nextChunkOffset = offset

guard offset > bytesHashed && !calculators.isEmpty else {
try await source.seek(to: offset) // <-- REWINDS THE UNDERLYING SOURCE BACK TO 32 MiB
return
}
...
```
`nextChunk = nil` discards the pre-fetched 32 MiB buffer, and `source.seek(to: 32MB)` rewinds the source.

3. **Double Reading on the Next Iteration**:
On the second iteration, `readChunk` starts over with `!isInitialized`, reading Chunk 1 (32..<64 MiB) **a second time** from the source! It then pre-fetches Chunk 2 (64..<96 MiB), which is discarded on the next 308 response, and the cycle repeats.

---

## Quantitative Impact

For any file requiring $K$ chunks (e.g. 128 MiB uploaded with 32 MiB chunks $\implies K = 4$):
- **Chunk 0** (0..<32 MiB): read 1 time (32 MiB)
- **Chunk 1** (32..<64 MiB): read **2 times** (64 MiB total)
- **Chunk 2** (64..<96 MiB): read **2 times** (64 MiB total)
- **Chunk 3** (96..<128 MiB): read **2 times** (64 MiB total)
- **Total data read from source**: $32 + 64 + 64 + 64 = \mathbf{224\text{ MiB}}$ for a 128 MiB object (**75% redundant reads**).

For a **1 GiB** file uploaded with 32 MiB chunks:
- Total data read from source: $32 + (31 \times 64) = \mathbf{2,016\text{ MiB}}$ (nearly **2x the file size**).

### Impact by Source Type:
- **`FileSource` (Local Disk / SSD)**: Every byte of the file after the first chunk is read from disk twice via `_NIOFileSystem`. Redundant disk reads saturate local storage bandwidth, increase disk I/O queue depth, and directly throttle upload throughput.
- **`BytesSource` (In-Memory Buffers)**: Slices and allocates multi-megabyte buffers twice, churning ARC reference counting, CPU L3 cache lines, and allocator heap structures.

---

## Proposed Solution

### 1. Evaluate `isLast` Directly When `totalSize` is Known
For `SeekableUploadSource` (e.g. `FileSource`, `BytesSource`), `source.totalSize` is known in advance.
Determine `isLast` directly without pre-fetching:
```swift
mutating func readChunk(maxBytes: Int) async throws -> ChunkInfo? {
if let totalSize = source.totalSize {
guard let chunk = try await source.read(maxBytes: maxBytes), !chunk.isEmpty else {
return nil
}
let currentChunkOffset = nextChunkOffset
nextChunkOffset += UInt64(chunk.count)
let isLast = nextChunkOffset >= totalSize
updateChecksums(data: chunk, startOffset: currentChunkOffset)
let checksumStr = isLast ? finalizeChecksum() : nil
return ChunkInfo(data: chunk, isLast: isLast, checksum: checksumStr)
}

// Fallback for unknown totalSize:
...
}
```

### 2. Happy-Path Progression Without Rewinding
In `StorageClient+Upload.swift` (`continueResumableSeekableUpload`):
When GCS responds with `HTTP 308` and `committedBytes == offset + chunk.count`, the stream is already positioned at `committedBytes`. Do not invoke `checksummedSource.seek(to:)`.
In `ChecksummedSource.seek(to:)`, if `offset == nextChunkOffset`, preserve `nextChunk` rather than clearing it and rewinding the source.

---

## Acceptance Criteria

- Uploading a 128 MiB file with 32 MiB chunks reads exactly 128 MiB from the source (verified by counting total bytes read on the underlying `UploadSource`).
- No backward seek operations are performed on the source during normal happy-path 308 progressions.
- Error recovery and resumption from partial chunks continue to function correctly when seeking backward.
- All storage upload tests pass with `-warnings-as-errors`.

Contributor guide

Open the contributing guide

Research direction

Start with ChecksummedSource.swift:102-117 and 146-154, then trace the resumable upload flow around StorageClient+Upload.swift:768 and continueResumableSeekableUpload. Verify how known totalSize, nextChunk, offsets, and backward seeks interact before changing the happy-path progression. Done means source bytes are read once, normal 308 progress makes no backward seeks, recovery still seeks correctly, and storage upload tests pass with -warnings-as-errors.

Written by the indexing model from the issue text.

Assessment

Tech stack
google-cloud, swift
Domain
api, cloud
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
65/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.