🧹 chore: Use sync.Pool for Client hooks#3758
🧹 chore: Use sync.Pool for Client hooks#3758gaby merged 8 commits intogofiber:mainfrom arturmelanchyk:file-buffer-pool
Conversation
|
Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
|
/gemini review |
|
@codex review |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3758 +/- ##
==========================================
- Coverage 91.44% 91.42% -0.02%
==========================================
Files 113 113
Lines 11824 11874 +50
==========================================
+ Hits 10812 10856 +44
- Misses 745 750 +5
- Partials 267 268 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR improves performance by introducing sync.Pool for buffer reuse in the parserRequestBodyFile function to reduce memory allocations when processing file uploads. The change shows significant performance improvements with 72.56% reduction in execution time and 99.58% reduction in bytes per operation.
- Replace direct buffer allocation with pooled buffer management using
sync.Pool - Add comprehensive benchmark test for measuring file parsing performance
- Introduce helper functions for benchmark request creation and cleanup
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| client/hooks.go | Implements sync.Pool for fileBuf to reuse 1MB buffers across requests |
| client/hooks_test.go | Adds benchmark test and helper functions to measure file parsing performance |
There was a problem hiding this comment.
Code Review
This pull request introduces a sync.Pool for file upload buffers, which significantly improves performance by reducing memory allocations, as demonstrated by the new benchmark. The implementation is correct and follows standard practices for using sync.Pool.
My review includes a couple of suggestions for the new benchmark code to align it more closely with the library's existing pooling patterns for File objects. This will improve test consistency and accuracy.
|
Codex Review: Didn't find any major issues. Nice work! About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback". |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a sync.Pool-backed 1MB buffer reuse for multipart file handling in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant Hooks as client/hooks.go
participant Pool as sync.Pool
participant Multipart as Multipart Builder
Caller->>Hooks: initiate multipart file handling
Hooks->>Pool: Get() -> *[]byte (1MB)
alt buffer acquired
Hooks->>Multipart: build/copy file data using buffer
Multipart-->>Hooks: multipart body assembled
Hooks->>Pool: Put(buffer) (deferred)
Hooks-->>Caller: proceed (success)
else buffer acquisition failed
Hooks-->>Caller: return error
end
note over Pool,Hooks: Buffer lifecycle: acquire → use → return
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/hooks.go (1)
283-304: Always close file readers on error to avoid FD leaksIf
io.CopyBufferorCreateFormFilefails,v.readerisn’t closed, leaking descriptors. Wrap per‑file work in a small closure and defer the close so it runs on both success and error.- // If reader is not set, open the file. - if v.reader == nil { - v.reader, err = os.Open(v.path) - if err != nil { - return fmt.Errorf("open file error: %w", err) - } - } - - // Create form file and copy the content. - w, err := mw.CreateFormFile(v.fieldName, v.name) - if err != nil { - return fmt.Errorf("create file error: %w", err) - } - - if _, err := io.CopyBuffer(w, v.reader, *fileBuf); err != nil { - return fmt.Errorf("failed to copy file data: %w", err) - } - - if err := v.reader.Close(); err != nil { - return fmt.Errorf("close file error: %w", err) - } + // Open, copy, and always close the reader (even on error). + if err := func(v *File) error { + if v.reader == nil { + r, err := os.Open(v.path) + if err != nil { + return fmt.Errorf("open file error: %w", err) + } + v.reader = r + } + r := v.reader + defer func() { _ = r.Close() }() + + w, err := mw.CreateFormFile(v.fieldName, v.name) + if err != nil { + return fmt.Errorf("create file error: %w", err) + } + if _, err := io.CopyBuffer(w, r, buf); err != nil { + return fmt.Errorf("failed to copy file data: %w", err) + } + return nil + }(v); err != nil { + return err + }
🧹 Nitpick comments (4)
client/hooks.go (3)
22-27: Simplify sync.Pool usage; drop pointer indirection and impossible error path
sync.Pool.Get()will invokeNewwhen empty, so the type assertion/len checks are unnecessary. Storing[]byte(not*[]byte) avoids an extra indirection and keeps the code simpler.var fileBufPool = sync.Pool{ - New: func() any { - b := make([]byte, 1<<20) // 1MB buffer - return &b - }, + New: func() any { + return make([]byte, 1<<20) // 1MB buffer + }, }- fileBuf, ok := fileBufPool.Get().(*[]byte) - if !ok || len(*fileBuf) == 0 { - return errors.New("failed to retrieve buffer from a sync.Pool") - } - - defer fileBufPool.Put(fileBuf) + buf := fileBufPool.Get().([]byte) + defer fileBufPool.Put(buf)- if _, err := io.CopyBuffer(w, v.reader, *fileBuf); err != nil { + if _, err := io.CopyBuffer(w, v.reader, buf); err != nil {Also applies to: 260-266, 297-297
5-5: Remove unused import after pool simplificationAfter dropping the defensive error path,
errorsbecomes unused.- "errors"
22-27: Optional: make buffer size a named constImproves discoverability/tuning in future perf work.
const fileCopyBufSize = 1 << 20 // 1MB // ... var fileBufPool = sync.Pool{ New: func() any { return make([]byte, fileCopyBufSize) } }client/hooks_test.go (1)
691-718: Report throughput with SetBytesExpose MB/s by setting processed bytes per op (sum of file payloads). Keeps comparison clearer.
b.ReportAllocs() for i := 0; i < b.N; i++ { + var totalBytes int64 + for _, c := range fileContents { + totalBytes += int64(len(c)) + } + b.SetBytes(totalBytes)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client/hooks.go(4 hunks)client/hooks_test.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Format Go code using gofumpt (enforced viamake format)
Ensure code passes golangci-lint checks (enforced viamake lint)
Optimize struct field alignment using betteralign (enforced viamake betteralign)
Modernize Go code using gopls modernize (enforced viamake modernize)
Files:
client/hooks.goclient/hooks_test.go
🧠 Learnings (2)
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
PR: gofiber/fiber#2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The project uses the testify/assert package for assertions in unit tests.
Applied to files:
client/hooks_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests in this project use testify require.
Applied to files:
client/hooks_test.go
🧬 Code graph analysis (2)
client/hooks.go (1)
client/client.go (1)
New(632-637)
client/hooks_test.go (1)
client/request.go (10)
Request(46-71)FormData(844-846)File(902-907)Header(678-680)AcquireFile(1002-1019)SetFileName(974-978)SetFileFieldName(981-985)SetFileReader(995-999)ReleaseRequest(963-966)ReleaseFile(1023-1026)
🔇 Additional comments (3)
client/hooks_test.go (3)
17-17: LGTM: fasthttp import for bench helpersImport is justified for Acquire/Release of raw request/args.
720-743: LGTM: request construction via poolsUsing AcquireFile and fasthttp Acquire* keeps the benchmark close to production pooling patterns.
745-751: LGTM: proper release of pooled resources — bench verification requiredReleasing Request, Args, and Files avoids skewing allocations; code change is correct. Sandbox couldn't run the provided bench/race checks due to errors ("go: cannot find main module" and "failed to start telemetry sidecar") — run the supplied script locally or in CI and attach the test output.
|
interesting, contradicts to linter requirement |
|
in the code I see that both approaches are used, but more common approach is to have pointer-like arguments with result check, so I'll keep my code as is to match this patter. |
|
@arturmelanchyk I'm pretty sure the pool should be without the pointer, at least in this case |
|
@gaby fixed |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
client/hooks.go (1)
5-5: Remove unused import after change
errorsbecomes unused with the simplified pool usage.- "errors"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client/hooks.go(3 hunks)client/hooks_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- client/hooks_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Format Go code using gofumpt (enforced viamake format)
Ensure code passes golangci-lint checks (enforced viamake lint)
Optimize struct field alignment using betteralign (enforced viamake betteralign)
Modernize Go code using gopls modernize (enforced viamake modernize)
Files:
client/hooks.go
🧬 Code graph analysis (1)
client/hooks.go (1)
client/client.go (3)
New(632-637)Get(691-693)Put(706-708)
🔇 Additional comments (1)
client/hooks.go (1)
14-14: LGTMImporting sync is required for the pool.
This reverts commit 5b71e69.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a sync.Pool for file buffers in parserRequestBodyFile to improve performance, which is a great optimization as demonstrated by the provided benchmarks. The implementation is correct and the addition of a benchmark test is excellent for verifying the improvements.
My review includes a few related suggestions to make the sync.Pool usage more idiomatic by pooling []byte directly instead of *[]byte. This is a minor refinement that improves code clarity and maintainability.
|
Congrats on merging your first pull request! 🎉 We here at Fiber are proud of you! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
Description
This PR improves performance by introducing
sync.PoolforfileBufinparserRequestBodyFilefunction$ benchstat old.txt sync-pool.txt goos: darwin goarch: arm64 pkg: github.com/gofiber/fiber/v3/client cpu: Apple M3 Max │ old.txt │ sync-pool.txt │ │ sec/op │ sec/op vs base │ _Parser_Request_Body_File 24.168µ ± 2% 6.661µ ± 1% -72.44% (p=0.000 n=20) │ old.txt │ sync-pool.txt │ │ B/s │ B/s vs base │ _Parser_Request_Body_File 3.788Gi ± 2% 13.745Gi ± 1% +262.83% (p=0.000 n=20) │ old.txt │ sync-pool.txt │ │ B/op │ B/op vs base │ _Parser_Request_Body_File 1028.138Ki ± 0% 4.095Ki ± 0% -99.60% (p=0.000 n=20) │ old.txt │ sync-pool.txt │ │ allocs/op │ allocs/op vs base │ _Parser_Request_Body_File 112.0 ± 0% 111.0 ± 0% -0.89% (p=0.000 n=20)Changes introduced
List the new features or adjustments introduced in this pull request. Provide details on benchmarks, documentation updates, changelog entries, and if applicable, the migration guide.
Type of change
Please delete options that are not relevant.
Checklist
Before you submit your pull request, please make sure you meet these requirements:
/docs/directory for Fiber's documentation.Commit formatting
Please use emojis in commit messages for an easy way to identify the purpose or intention of a commit. Check out the emoji cheatsheet here: CONTRIBUTING.md