refactor: replace Split in loops with more efficient SplitSeq#7278
refactor: replace Split in loops with more efficient SplitSeq#7278stringsbuilder wants to merge 4 commits into
Conversation
Signed-off-by: stringsbuilder <stringsbuilder@outlook.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR migrates string iteration patterns in integration tests from eager-evaluated ChangesString Sequence Iterator Refactoring
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/protocols/network/network.go (1)
202-213:⚠️ Potential issue | 🟡 MinorTrim whitespace before parsing each port token.
Line 206 will reject a common input like
port: "80, 443"because the second token still includes leading whitespace.✂️ Suggested fix
- for port := range strings.SplitSeq(request.Port, ",") { + for rawPort := range strings.SplitSeq(request.Port, ",") { + port := strings.TrimSpace(rawPort) if port == "" { continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/protocols/network/network.go` around lines 202 - 213, When iterating tokens from strings.SplitSeq(request.Port, ",") trim whitespace from each token before using it: use a local variable (e.g., tok := strings.TrimSpace(port)), skip if tok == "", parse tok with strconv.Atoi, validate its range, and append the trimmed token (tok) to request.ports (not the original port with whitespace). Update the error messages to include tok where appropriate so parsing failures reflect the trimmed value; keep references to request.Port, strings.SplitSeq, strconv.Atoi, and request.ports.
🧹 Nitpick comments (2)
pkg/tmplexec/flow/flow_executor.go (1)
302-311: This still materializes the entire helper file before tokenization.Line 302's
io.ReadAll(reader)dominates the allocation profile here, and the substrings appended in Lines 306-310 keep that full backing string alive. If this path is part of the perf goal, scan the reader directly instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/tmplexec/flow/flow_executor.go` around lines 302 - 311, The code currently calls io.ReadAll(reader) and then iterates over strings.SplitSeq(string(bin), "\n"), which materializes the entire reader into memory and keeps the large backing string alive; replace that pattern by scanning the reader directly with a bufio.Scanner (or bufio.Reader.ReadString/ReadBytes loop) to read lines one-by-one from reader, trim each scanned line (strings.TrimSpace), and append non-empty lines to values, and if necessary configure Scanner.Buffer to handle long lines; update the logic that references reader, values, and strings.SplitSeq to use the scanner instead.pkg/installer/template.go (1)
568-575: There's still a per-record split allocation in this loop.Line 570 only needs two fields, but the full split still creates a slice for every checksum entry. A two-part split would align better with the allocation-reduction goal of this PR.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/installer/template.go` around lines 568 - 575, The per-record allocation comes from using strings.Split on each checksum line (variable v) and assigning to tmparr; replace that with a two-field split (e.g., strings.SplitN(v, ",", 2) or better strings.Cut(v, ",")) to avoid allocating a full slice for every entry, then validate the second field exists before assigning into allChecksums[tmparr[0]] = tmparr[1] (or using the two return values from strings.Cut) while keeping the surrounding loop and trimming logic the same.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@pkg/protocols/network/network.go`:
- Around line 202-213: When iterating tokens from strings.SplitSeq(request.Port,
",") trim whitespace from each token before using it: use a local variable
(e.g., tok := strings.TrimSpace(port)), skip if tok == "", parse tok with
strconv.Atoi, validate its range, and append the trimmed token (tok) to
request.ports (not the original port with whitespace). Update the error messages
to include tok where appropriate so parsing failures reflect the trimmed value;
keep references to request.Port, strings.SplitSeq, strconv.Atoi, and
request.ports.
---
Nitpick comments:
In `@pkg/installer/template.go`:
- Around line 568-575: The per-record allocation comes from using strings.Split
on each checksum line (variable v) and assigning to tmparr; replace that with a
two-field split (e.g., strings.SplitN(v, ",", 2) or better strings.Cut(v, ","))
to avoid allocating a full slice for every entry, then validate the second field
exists before assigning into allChecksums[tmparr[0]] = tmparr[1] (or using the
two return values from strings.Cut) while keeping the surrounding loop and
trimming logic the same.
In `@pkg/tmplexec/flow/flow_executor.go`:
- Around line 302-311: The code currently calls io.ReadAll(reader) and then
iterates over strings.SplitSeq(string(bin), "\n"), which materializes the entire
reader into memory and keeps the large backing string alive; replace that
pattern by scanning the reader directly with a bufio.Scanner (or
bufio.Reader.ReadString/ReadBytes loop) to read lines one-by-one from reader,
trim each scanned line (strings.TrimSpace), and append non-empty lines to
values, and if necessary configure Scanner.Buffer to handle long lines; update
the logic that references reader, values, and strings.SplitSeq to use the
scanner instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bb4462ab-2d3f-4a47-917f-e710b053336f
📒 Files selected for processing (10)
cmd/integration-test/http.gointernal/runner/options_test.gopkg/catalog/config/nucleiconfig.gopkg/catalog/config/template.gopkg/installer/template.gopkg/js/devtools/tsgen/parser.gopkg/protocols/javascript/js.gopkg/protocols/network/network.gopkg/testutils/integration.gopkg/tmplexec/flow/flow_executor.go
Neo - PR Security ReviewNo security issues found Highlights
Hardening Notes
Comment |
themavik
left a comment
themavik
left a comment
There was a problem hiding this comment.
SplitSeq/FieldsSeq churn is straightforward. nit: a few call sites still use strings.Split for small inner splits (e.g. installer checksum line) — not wrong, just inconsistent if you are chasing allocs everywhere.
|
@Mzack9999 @dogancanbakir Hi, Could you please review this PR at your convenience? Thank you very much. |
|
Holding review until #7419 is merged. The |
3 participants
Proposed changes
strings.SplitSeq (introduced in Go 1.23) returns a lazy sequence (strings.Seq), allowing gopher to iterate over tokens one by one without creating an intermediate slice.
It significantly reduces memory allocations and can improve performance for long strings.
More info: golang/go#61901
Proof
Checklist
Summary by CodeRabbit