Conversation
There was a problem hiding this comment.
Pull request overview
This PR optimizes download performance by eliminating lock contention in the multi-part download progress tracking system. The key improvement is replacing mutex-protected progress updates with lock-free atomic operations, which significantly reduces contention when multiple download goroutines are running concurrently.
- Introduces cache-line padded atomic counters to prevent false sharing between CPU cores
- Separates progress bar UI updates from download goroutines using a dedicated ticker
- Optimizes I/O with 1MB buffers and connection pool tuning
- Removes
pull_requesttrigger from release workflow to prevent unnecessary runs
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| main.go | Implements lock-free progress tracking with atomic counters, adds UI update ticker goroutine, optimizes buffer usage with io.CopyBuffer, and disables HTTP/2 for better connection distribution |
| .github/workflows/release.yml | Removes pull_request trigger and adds redundant tag check to ensure releases only run on tag pushes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| finalTotal += atomic.LoadUint64(&dl.partDownloaded[i].val) | ||
| } | ||
| if diff := finalTotal - lastTotal; diff > 0 { | ||
| bar.Add64(int64(diff)) |
There was a problem hiding this comment.
The final progress bar update on context cancellation doesn't update lastTotal, which could lead to inconsistent state. Consider updating lastTotal = finalTotal after calling bar.Add64() to maintain consistency with the ticker case.
| bar.Add64(int64(diff)) | |
| bar.Add64(int64(diff)) | |
| lastTotal = finalTotal |
There was a problem hiding this comment.
Doesn't it return immediately after and lastTotal being a locally scoped var become out of scope?
| // Use 1MB buffer to reduce syscall overhead | ||
| buf := make([]byte, 1024*1024) |
There was a problem hiding this comment.
A 1MB buffer is allocated per goroutine (one per download part). With the default 8 parts, this allocates 8MB. Consider using a sync.Pool to reuse buffers across parts, which would reduce memory allocation overhead and GC pressure during downloads.
| // atomicCounter is padded to 64 bytes to prevent false sharing between CPU cores. | ||
| // Without padding, adjacent counters share a cache line, causing cache thrashing. | ||
| type atomicCounter struct { | ||
| val uint64 | ||
| _ [56]byte // pad to 64 bytes (cache line size) | ||
| } |
There was a problem hiding this comment.
The new atomicCounter type and lock-free progress tracking implementation lack test coverage. Consider adding tests that verify: 1) cache line padding is correct (64 bytes), 2) concurrent access to atomic counters doesn't cause data races, and 3) progress bar updates correctly reflect concurrent writes from multiple goroutines.
| // UI update ticker - updates progress bar without mutex contention in download loop | ||
| go func() { | ||
| uiTicker := time.NewTicker(100 * time.Millisecond) | ||
| defer uiTicker.Stop() | ||
|
|
||
| var lastTotal uint64 | ||
| for { | ||
| select { | ||
| case <-ctx.Done(): | ||
| // Final update to ensure bar reaches 100% | ||
| var finalTotal uint64 | ||
| for i := range dl.partDownloaded { | ||
| finalTotal += atomic.LoadUint64(&dl.partDownloaded[i].val) | ||
| } | ||
| if diff := finalTotal - lastTotal; diff > 0 { | ||
| bar.Add64(int64(diff)) | ||
| } | ||
| return | ||
| case <-uiTicker.C: | ||
| var currentTotal uint64 | ||
| for i := range dl.partDownloaded { | ||
| currentTotal += atomic.LoadUint64(&dl.partDownloaded[i].val) | ||
| } | ||
| if diff := currentTotal - lastTotal; diff > 0 { | ||
| bar.Add64(int64(diff)) | ||
| lastTotal = currentTotal | ||
| } | ||
| } | ||
| } | ||
| }() |
There was a problem hiding this comment.
The UI update ticker goroutine lacks test coverage. Consider adding tests to verify: 1) progress bar updates correctly aggregate atomic counter values, 2) the final update on context cancellation completes properly, and 3) the ticker handles edge cases like zero progress or rapid completion.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
No description provided.