♻️ refactor: Optimize string handling and memory allocations #3922
Conversation
|
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. WalkthroughThis PR applies targeted performance and parsing refinements across many files: replaces allocation-heavy string operations with Cut/SplitSeq or Index/Count, refactors viewBindMap from sync.Map to Map, adds helper functions and tests, and updates comments/linter annotations. No exported API changes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
Summary of ChangesHello @ReneWerner87, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on a comprehensive refactoring effort to enhance the application's performance and reduce its memory footprint. The changes primarily involve optimizing string manipulation techniques by leveraging more efficient Go standard library functions and patterns that minimize heap allocations. Additionally, data structures within the context handling were streamlined for better performance, and HTTP header parsing logic was made more robust and RFC-compliant. These improvements contribute to a more efficient and responsive system. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3922 +/- ##
==========================================
+ Coverage 91.60% 91.67% +0.07%
==========================================
Files 119 119
Lines 10096 10117 +21
==========================================
+ Hits 9248 9275 +27
+ Misses 539 534 -5
+ Partials 309 308 -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:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several performance optimizations by replacing strings.Split with more efficient alternatives like strings.Count, strings.Cut, and strings.SplitSeq, and by pre-allocating maps to reduce memory allocations. The change from sync.Map to a standard map for viewBindMap is also a good improvement for the context's performance. Most changes are well-implemented and improve efficiency.
However, I've found a critical bug in the new headerContainsValue helper function in res.go. The implementation can fail to correctly detect existing values in comma-separated headers if they are not space-separated, which could lead to incorrect header values being constructed. I've provided a detailed comment and a suggested fix for this issue.
Apart from that, the refactoring is solid and improves the codebase.
There was a problem hiding this comment.
Pull request overview
This PR optimizes string handling and memory allocations across various functions in the codebase to improve performance and reduce unnecessary heap allocations.
- Replaces
sync.MapwithMap(map[string]any) forviewBindMapto simplify the code and improve performance - Optimizes string splitting operations using
strings.Cut,strings.SplitSeq, and direct slicing to avoid allocations - Adds
headerContainsValuehelper function with optimized string matching for HTTP header values - Pre-allocates maps with known capacity to avoid reallocations
- Uses stack-allocated arrays for common cases to avoid heap allocations
- Improves nolint comments to be more specific about the ignored gosec warnings
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| state_test.go | Updated nolint comments to be more descriptive about why G115 warnings are safe to ignore in test cases |
| router.go | Updated nolint comments for atomic operations to clarify why G115 warnings are safe |
| res.go | Added headerContainsValue helper function and pre-allocated map in GetHeaders with known capacity |
| req.go | Pre-allocated map in GetHeaders and optimized Subdomains with stack-allocated array using strings.SplitSeq |
| mount.go | Updated nolint comment for handler count to be more descriptive |
| middleware/static/static.go | Optimized wildcard prefix handling using direct string slicing instead of strings.Split |
| middleware/cache/cache.go | Updated nolint comments for Unix timestamp conversions to be more descriptive |
| middleware/adaptor/adaptor.go | Added blank line to documentation comment for proper formatting |
| helpers_test.go | Added comprehensive tests for headerContainsValue and expanded isNoCache tests |
| helpers.go | Optimized acceptsLanguageOfferExtended with stack-allocated arrays and enhanced isNoCache to support RFC 9111 no-cache with field-name arguments |
| ctx_test.go | Updated nolint comment to be more descriptive about the test value range |
| ctx.go | Replaced sync.Map with Map for viewBindMap, simplified iteration logic, and improved memory management |
| client/hooks.go | Optimized URL parsing using strings.Cut to avoid unnecessary allocations |
| client/client_test.go | Refactored hook test to use named function variables for better readability |
| binder/mapping.go | Optimized tag parsing with strings.Cut and value splitting with strings.SplitSeq |
| app.go | Optimized path segment counting using strings.Count instead of splitting |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
res.go (1)
136-154: headerContainsValue helper matches Append’s canonical formattingCentralizing the duplicate-check logic into
headerContainsValuekeepsAppendtight and makes the matching rules explicit. The helper:
- Handles exact, prefix (
"value,..."), suffix ("... , value"), and middle ("..., value, ..."with a leading space) positions.- Intentionally does not treat compact forms like
"deflate,gzip,br"as containing"gzip", which matches the new tests and Fiber’s own", "-separated formatting.The trade-off is that if a response header was manually set in a non-canonical form,
Appendmay now allow a duplicate token (e.g."deflate,gzip,br, gzip"). That’s harmless for semantics but worth being aware of.Also applies to: 156-179
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
app.go(1 hunks)binder/mapping.go(2 hunks)client/client_test.go(1 hunks)client/hooks.go(3 hunks)ctx.go(4 hunks)ctx_test.go(1 hunks)helpers.go(4 hunks)helpers_test.go(2 hunks)middleware/adaptor/adaptor.go(1 hunks)middleware/cache/cache.go(3 hunks)middleware/static/static.go(1 hunks)mount.go(1 hunks)req.go(2 hunks)res.go(3 hunks)router.go(4 hunks)state_test.go(8 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
When adding Go tests, always invoke
t.Parallel()at the start of each test and subtest to maximize concurrency
Files:
state_test.goctx_test.gohelpers_test.goclient/client_test.go
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Prefer
github.com/gofiber/utils/v2helpers (for example,utils.Trim) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code
Files:
state_test.gomiddleware/static/static.goctx_test.gomount.gorouter.gohelpers_test.goreq.gomiddleware/adaptor/adaptor.gomiddleware/cache/cache.goclient/client_test.goapp.gobinder/mapping.goclient/hooks.goctx.gores.gohelpers.go
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
Repo: gofiber/fiber PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T13:34:08.100Z
Learning: Applies to **/*.go : Prefer `github.com/gofiber/utils/v2` helpers (for example, `utils.Trim`) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code
📚 Learning: 2025-10-16T07:19:52.418Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:19:52.418Z
Learning: In the Fiber codebase, the linter does not allow `require` assertions from within HTTP handlers (including net/http-style handlers). Use `t.Fatalf`, `t.Errorf`, or similar `testing.T` methods for error handling inside handler functions instead.
Applied to files:
ctx_test.goctx.go
📚 Learning: 2025-10-16T07:15:26.529Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:15:26.529Z
Learning: In Fiber v3, net/http handlers (http.Handler, http.HandlerFunc, or raw func(http.ResponseWriter, *http.Request)) can be passed directly to routing methods like app.Get(), app.Post(), etc. The framework automatically detects and wraps them internally via toFiberHandler/collectHandlers. The github.com/gofiber/fiber/v3/middleware/adaptor package is legacy and should not be suggested for tests or code using native net/http handler support.
Applied to files:
mount.gorouter.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In the Fiber framework tests, using `ctx.Response.Header.Cookie` may not be suitable for parsing cookies from the response header, as it requires a `*Cookie` and fills it rather than returning a string value; thus, manual parsing of the `Set-Cookie` header may be necessary.
Applied to files:
helpers_test.gomiddleware/cache/cache.go
📚 Learning: 2024-11-10T23:44:13.704Z
Learnt from: gaby
Repo: gofiber/fiber PR: 3193
File: middleware/adaptor/adaptor.go:111-111
Timestamp: 2024-11-10T23:44:13.704Z
Learning: In the `middleware/adaptor/adaptor.go` file of the Fiber framework, when updating context handling, replacing `c.Context()` with `c.RequestCtx()` is appropriate to access the `fasthttp.RequestCtx`.
Applied to files:
middleware/adaptor/adaptor.go
📚 Learning: 2024-11-08T04:10:42.990Z
Learnt from: gaby
Repo: gofiber/fiber PR: 3193
File: middleware/cache/cache_test.go:897-897
Timestamp: 2024-11-08T04:10:42.990Z
Learning: In the Fiber framework, `Context()` is being renamed to `RequestCtx()`, and `UserContext()` to `Context()` to improve clarity and align with Go's context conventions.
Applied to files:
middleware/adaptor/adaptor.goctx.go
📚 Learning: 2024-11-15T07:56:21.623Z
Learnt from: ReneWerner87
Repo: gofiber/fiber PR: 3161
File: app.go:923-932
Timestamp: 2024-11-15T07:56:21.623Z
Learning: In the Fiber framework, breaking changes are acceptable when moving from version 2 to version 3, including modifications to method signatures such as in the `Test` method in `app.go`.
Applied to files:
middleware/adaptor/adaptor.go
📚 Learning: 2025-11-26T13:34:08.100Z
Learnt from: CR
Repo: gofiber/fiber PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T13:34:08.100Z
Learning: Applies to **/*.go : Prefer `github.com/gofiber/utils/v2` helpers (for example, `utils.Trim`) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code
Applied to files:
ctx.go
🧬 Code graph analysis (4)
req.go (1)
client/request.go (2)
Request(46-73)Header(708-710)
client/client_test.go (2)
client/client.go (1)
Client(37-67)client/response.go (1)
Response(19-25)
ctx.go (1)
app.go (1)
Map(43-43)
res.go (3)
client/response.go (1)
Response(19-25)client/request.go (1)
Header(708-710)bind_test.go (2)
Header(331-335)Header(574-578)
🪛 GitHub Actions: Modernize Lint
ctx.go
[error] 325-325: Replace m[k]=v loop with maps.Copy.
🪛 GitHub Check: modernize
ctx.go
[failure] 325-325:
Replace m[k]=v loop with maps.Copy
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Agent
- GitHub Check: Compare
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: unit (1.25.x, macos-latest)
- GitHub Check: repeated
- GitHub Check: lint
🔇 Additional comments (19)
state_test.go (1)
987-987: G115 annotations in benchmarks/tests look correct.Explicit nolint notes for small, bounded casts make the intent clear and avoid noisy false positives. LGTM.
Also applies to: 1004-1004, 1021-1021, 1038-1038, 1072-1072, 1090-1090, 1107-1107, 1123-1123
middleware/adaptor/adaptor.go (1)
71-71: Minor doc spacing change acknowledged.Spacer before the Deprecated note is fine; no behavioral impact.
middleware/cache/cache.go (1)
94-94: Clear G115 justifications for conversions.The added comments document safe bounds for these uint64 casts (Unix seconds, Age). Good clarity without changing behavior.
Also applies to: 107-107, 305-305
mount.go (1)
187-187: G115 note on handler count cast is appropriate.Route handlers are bounded; documenting the safe uint32 cast is sufficient.
ctx_test.go (1)
6435-6435: Reasonable G115 suppression in test.Casting lineNum (1–5) to int32 is safe; the nolint note clarifies intent for gosec.
app.go (1)
1305-1315: Slash-counting for mount prefix depth is correct and cheaperUsing
strings.Count(prefix, "/") + 1to derivepartspreserves the previous segment-count semantics for normalized prefixes while avoiding allocations fromSplit. Givenprefix != ""and mount prefixes are already normalized, this is a safe perf win.router.go (1)
472-481: Atomic handler-count adjustments remain consistentThe added
//nolint:gosec // G115 - handler count is always smallcomments simply document the existing bitwise decrement/increment pattern. The atomic updates still correctly mirror route registration and removal acrossdeleteRoute,pruneAutoHeadRouteLocked,register, andensureAutoHeadRoutesLocked, so no behavioral change is introduced.Also applies to: 506-510, 571-577, 673-677
client/client_test.go (1)
286-303: Refactoring response hooks into named closures is fineExtracting the two response hooks into
hook1/hook2and passing them toAddResponseHookkeeps the semantics identical while improving readability. Test structure (includingt.Parallel()) remains correct.res.go (1)
440-452: GetHeaders optimization via Response.Header.All is appropriateUsing
respHeader.Len()to size the map and iteratingrespHeader.All()avoids intermediate allocations and keeps key/value lifetimes within the documented handler scope. Converting the iterated[]bytekey/value pairs tostringbefore storing in the map maintains the existing contract ofGetHeaders()while reducing per-call overhead.helpers_test.go (1)
637-666: Additional cache-control and headerContainsValue tests align with helper semanticsThe new
isNoCachecases (argument forms likeno-cache="Set-Cookie"and spacing variants) accurately exercise the expanded parsing inhelpers.go. Likewise,Test_HeaderContainsValueand its benchmark cover the exact/prefix/suffix/middle and empty/similar-token scenarios thatheaderContainsValueis designed for, guarding against regressions in response-header deduplication.Also applies to: 683-731
helpers.go (2)
222-278: acceptsLanguageOfferExtended: SplitSeq-based parsing looks correct and efficientRefactoring
acceptsLanguageOfferExtendedto:
- Use small stack buffers (
[8]string) withstrings.SplitSeqfor spec/offer subtags, and- Keep the same sliding/wildcard/singleton-barrier logic,
maintains the RFC 4647 extended filtering behavior while reducing heap churn for typical language tags. Tests such as
"de-*-DE"vs"de-Latn-DE-1996"and"en-*"vs"en"still pass under this implementation.
739-769: Expanded isNoCache handling correctly covers directive-with-argumentsThe new
isNoCacheimplementation:
- Anchors on case-insensitive
no-cachetokens with proper delimiter checks on the left (start, space, or comma).- Treats end-of-string, comma,
=, or space immediately afterno-cacheas a valid directive boundary, so forms like"no-cache","no-cache=Set-Cookie", and"no-cache ,public"are all accepted.- Continues to reject substrings like
"Xno-cache"and"no-cacheX".This matches the updated tests and RFC 9111’s directive forms while avoiding spurious matches.
middleware/static/static.go (1)
118-122: Wildcard prefix truncation via Index is a clean micro-optimizationSwitching to
if idx := strings.Index(prefix, "*"); idx != -1 { prefix = prefix[:idx] }preserves the previous behavior for partial wildcards (e.g."/john*"→"/john") while avoiding the extra slice allocations fromstrings.Split. The subsequentprefixLenhandling is unchanged.ctx.go (1)
62-74: viewBindMap refactor to Map using maps.Copy is correctly implementedThe refactor from
sync.Mapto theMapalias (map[string]any) is safe and properly implemented:
viewBindMapfield (line 62) is now typeMap, eliminating unnecessary synchronization overheadViewBind()method lazily initializes the map and usesmaps.Copy()from the stdlib to copy entries, replacing the manualfor k, v := range varslooprelease()method (line 608) resets the map withclear(c.viewBindMap), preserving capacity for reuse across pooled contextsrenderExtensions()method directly iterates overc.viewBindMapto merge view bindings without sync overheadThis refactoring is safe because
DefaultCtxis request-scoped and reused via an object pool with no concurrent goroutine access to the same instance. The stdlibmapspackage (Go 1.21+) is the correct choice here—it is idiomatic, satisfies the modernize linter, andgofiber/utils/v2provides no map copy utility. The coding guideline for preferringgofiber/utils/v2helpers applies specifically to string manipulation operations, not map operations.binder/mapping.go (2)
214-221: Struct tag parsing viastrings.Cutkeeps behavior while avoiding extra allocUsing
strings.Cutto strip options from the tag (Line 217) preserves the previousSplit(...)[0]semantics, including edge cases with empty segments, and keeps the path usingutils.ToLowerconsistent with existing style. This is a safe, allocation‑friendly change.Based on learnings, this also keeps tag handling aligned with our utils‑first string conventions where applicable.
379-386: SplitSeq-based slice binding preserves semantics and cuts allocationsThe new
assignBindDataloop overstrings.SplitSeq(Lines 381‑383) still only activates when splitting is enabled, the value actually contains a comma, and the target field is a slice; the resulting elements are appended one by one, matching the former behavior while avoiding the intermediate[]stringallocation. Looks good for both correctness and performance.req.go (2)
303-311: Header map preallocation withRequestHeader.Allis correct and efficientUsing a local
reqHeaderplusmake(map[string][]string, reqHeader.Len())and iterating viareqHeader.All()(Lines 304‑307) correctly captures all headers, including multi‑value ones, while reducing reallocations. Converting keys/values throughapp.toStringalso respects fasthttp’s iterator lifetime constraints.
837-897: Subdomains stack-buffer +SplitSeqrefactor maintains behavior and avoids leaksThe new
partsBuf/partspattern withstrings.SplitSeq(Lines 871‑878) avoids a heap slice in the common ≤8‑label case, and the function now always returns heap‑allocated copies (result) for bothoffset == 0and trimmed cases (Lines 883‑885, 893‑896), so no stack‑backed data escapes. Existing semantics—default offset 2, empty for IPs or excessive offset, normalized and punycode‑decoded host—are preserved.Based on learnings, this continues to pair well with existing utils helpers like
utils.TrimRightandutils.IsIPv4/IsIPv6.client/hooks.go (1)
73-75: URL parsing withstrings.Cutsimplifies path/query/hash handling without changing behaviorSwitching to
strings.Cutfor separating the base URI from?and then query from#(Lines 74, 101) removes the intermediate slice fromSplit, while still:
- Normalizing and templating the full URI before setting it on the request.
- Parsing only the true query portion into
fasthttp.Args(Line 105).- Explicitly setting the hash fragment via
SetHash(hashPart)(Line 115).This keeps existing semantics for URLs with and without query/fragment while tightening allocations in a hot path.
Also applies to: 100-106, 115-116
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…rocessing and escape character removal
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
helpers_test.go (1)
683-736: HeaderContainsValue tests & benchmark look solid; consider case-sensitivity needsThe new table-driven tests cover exact token matches at start/middle/end, OWS variations, and substring false-positives, which is exactly what this helper needs for comma-separated headers. The benchmark mirrors realistic patterns and asserts a true result to keep the compiler honest; plus
t.Parallel()is correctly used on the new test.If
headerContainsValueis intended to treat values case-insensitively for specific headers (e.g.,Accept-Encodingtokens), you might optionally add a couple of mixed-case cases (like"GZip"vs"gzip") to document that behavior explicitly; otherwise this is good as-is. As per coding guidelines, the use oft.Parallel()here matches the test-concurrency requirement.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
ctx.go(6 hunks)helpers_test.go(2 hunks)path.go(2 hunks)res.go(3 hunks)router.go(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- router.go
- res.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Prefer
github.com/gofiber/utils/v2helpers (for example,utils.Trim) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code
Files:
path.goctx.gohelpers_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
When adding Go tests, always invoke
t.Parallel()at the start of each test and subtest to maximize concurrency
Files:
helpers_test.go
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
Repo: gofiber/fiber PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T13:34:08.100Z
Learning: Applies to **/*.go : Prefer `github.com/gofiber/utils/v2` helpers (for example, `utils.Trim`) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code
📚 Learning: 2024-11-15T07:56:21.623Z
Learnt from: ReneWerner87
Repo: gofiber/fiber PR: 3161
File: app.go:923-932
Timestamp: 2024-11-15T07:56:21.623Z
Learning: In the Fiber framework, breaking changes are acceptable when moving from version 2 to version 3, including modifications to method signatures such as in the `Test` method in `app.go`.
Applied to files:
path.go
📚 Learning: 2025-11-26T13:34:08.100Z
Learnt from: CR
Repo: gofiber/fiber PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T13:34:08.100Z
Learning: Applies to **/*.go : Prefer `github.com/gofiber/utils/v2` helpers (for example, `utils.Trim`) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code
Applied to files:
path.goctx.go
📚 Learning: 2024-11-10T23:44:13.704Z
Learnt from: gaby
Repo: gofiber/fiber PR: 3193
File: middleware/adaptor/adaptor.go:111-111
Timestamp: 2024-11-10T23:44:13.704Z
Learning: In the `middleware/adaptor/adaptor.go` file of the Fiber framework, when updating context handling, replacing `c.Context()` with `c.RequestCtx()` is appropriate to access the `fasthttp.RequestCtx`.
Applied to files:
ctx.go
📚 Learning: 2024-11-08T04:10:42.990Z
Learnt from: gaby
Repo: gofiber/fiber PR: 3193
File: middleware/cache/cache_test.go:897-897
Timestamp: 2024-11-08T04:10:42.990Z
Learning: In the Fiber framework, `Context()` is being renamed to `RequestCtx()`, and `UserContext()` to `Context()` to improve clarity and align with Go's context conventions.
Applied to files:
ctx.go
📚 Learning: 2025-10-16T07:19:52.418Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:19:52.418Z
Learning: In the Fiber codebase, the linter does not allow `require` assertions from within HTTP handlers (including net/http-style handlers). Use `t.Fatalf`, `t.Errorf`, or similar `testing.T` methods for error handling inside handler functions instead.
Applied to files:
ctx.go
🧬 Code graph analysis (1)
ctx.go (2)
app.go (1)
Map(43-43)constants.go (1)
HeaderXRequestedWith(282-282)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: unit (1.25.x, macos-latest)
- GitHub Check: repeated
- GitHub Check: Compare
- GitHub Check: lint
🔇 Additional comments (8)
helpers_test.go (1)
653-660: IsNoCache: RFC 9111 field-name and spacing cases covered wellThe added cases for
no-cachewith field-name arguments (quoted and unquoted) and odd spacing nicely pin down the RFC 9111 behaviors and OWS handling. The expectations all look consistent with the usual “any no-cache directive implies no-cache” semantics.path.go (2)
623-640: LGTM! Excellent optimization.The fast path avoids allocation when no escape characters are present, and the slow path efficiently starts copying from the first escape index. The logic is correct and should significantly improve performance for strings without escapes.
707-719: LGTM! Efficient constraint validation.The switch-based validation eliminates slice allocation and clearly groups constraints by their data requirements. The logic correctly validates that single-data constraints have at least one element and two-data constraints have at least two elements.
ctx.go (5)
322-326: LGTM! Cleaner map initialization.Using
make(Map, len(vars))with capacity hint andmaps.Copyis more efficient and readable than individualStorecalls. The standard library function handles the copy correctly.
537-544: LGTM! Efficient XHR detection.Precomputing
xmlHTTPRequestByteseliminates repeated allocations, and usingutils.EqualFoldwith the precomputed byte slice for case-insensitive comparison is the right approach.
625-629: LGTM! Correct map iteration.The standard map range iteration correctly replaces the
sync.Map.Rangemethod, maintaining the same logic of copying keys that don't already exist inbindMap.
63-63: The sync.Map to Map change is safe and improves performance.The
viewBindMapfield change fromsync.Mapto regularMapis correct. Each request acquires a unique context instance from the pool (AcquireCtx) and releases it after request handling (ReleaseCtx), withdeferstatements preventing concurrent access. The viewBindMap is only accessed within the request lifecycle—once for population viamaps.CopyinViewBind()and once for reading inrenderExtensions(). No concurrent goroutine access to the same context instance occurs, making the synchronization overhead ofsync.Mapunnecessary.
610-611: LGTM! Efficient map reuse.Using
clear(c.viewBindMap)instead of reinitializing preserves the underlying map allocation for reuse, which is more efficient. Theclearbuilt-in (Go 1.21+) is fully compatible with Fiber v3's Go 1.25 requirement.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a series of excellent performance optimizations and correctness fixes across various parts of the codebase. The changes effectively reduce memory allocations and improve string handling efficiency by leveraging modern Go features and performance patterns, such as using strings.Cut, strings.SplitSeq, stack-allocated buffers, and pre-computed constants. The refactoring from sync.Map to a standard map for viewBindMap is a good choice for the request context lifecycle. Additionally, the correctness fixes for header parsing logic in isNoCache and the new headerContainsValue function are valuable improvements. Overall, this is a high-quality contribution that enhances the framework's performance and robustness.
|
Why sync.Map was replaced with a regular Map
|
|
@gofiber/maintainers In addition to the AI review, I need a human review here, as quite a lot has been adjusted, just to be on the safe side. |
|
@codex review |
gaby
changed the title
|
Codex Review: Didn't find any major issues. Keep it up! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
ReneWerner87
deleted the
optimize-string-handling-and-memory-allocations-in-various-functions
branch
No description provided.