Skip to content

perf(core/stringx): replace manual char filter with strings.Map#5453

Merged
kevwan merged 1 commit intozeromicro:masterfrom
1911860538:core/stringx
Mar 22, 2026
Merged

perf(core/stringx): replace manual char filter with strings.Map#5453
kevwan merged 1 commit intozeromicro:masterfrom
1911860538:core/stringx

Conversation

@1911860538
Copy link
Contributor

The original Filter function used a manual []rune loop to remove characters
satisfying a predicate. This change:

  • Replaces it with a simpler, more efficient implementation using strings.Map
  • Renames the predicate parameter from 'filter' to 'remove' to accurately
    reflect that the function removes characters for which remove(r) is true

@1911860538
Copy link
Contributor Author

Below are benchmark results:

Before

goos: darwin
goarch: amd64
pkg: github.com/zeromicro/go-zero/core/stringx
cpu: Intel(R) Core(TM) i7-8569U CPU @ 2.80GHz
BenchmarkFilter
BenchmarkFilter/true
BenchmarkFilter/true-8         	12483945	        84.49 ns/op	      16 B/op	       1 allocs/op
BenchmarkFilter/false
BenchmarkFilter/false-8        	12183482	        86.93 ns/op	      16 B/op	       1 allocs/op
PASS

Process finished with the exit code 0

After

goos: darwin
goarch: amd64
pkg: github.com/zeromicro/go-zero/core/stringx
cpu: Intel(R) Core(TM) i7-8569U CPU @ 2.80GHz
BenchmarkFilter
BenchmarkFilter/true
BenchmarkFilter/true-8         	15771994	        68.64 ns/op	      16 B/op	       1 allocs/op
BenchmarkFilter/false
BenchmarkFilter/false-8        	40227080	        30.35 ns/op	       0 B/op	       0 allocs/op
PASS

Process finished with the exit code 0

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates core/stringx.Filter to use strings.Map for rune removal and adds a benchmark to evaluate the change.

Changes:

  • Reimplemented Filter using strings.Map and renamed the predicate parameter from filter to remove (doc/clarity).
  • Added BenchmarkFilter to measure performance and allocations for always-remove vs never-remove predicates.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
core/stringx/strings.go Switches Filter implementation to strings.Map and updates the parameter name/comment for clarity.
core/stringx/strings_test.go Adds a benchmark intended to measure the new Filter behavior in two scenarios.

@codecov
Copy link

codecov bot commented Mar 15, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Copy link
Contributor

@kevwan kevwan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review

Semantics — The predicate semantics are preserved: both old filter(r) == true (character was excluded via if !filter(x) guard) and new remove(r) == true mean "drop this rune". Renaming the parameter from filter to remove makes the intent clearer. ✓

Correctnessstrings.Map(func(r rune) rune { if remove(r) { return -1 }; return r }, s) is the canonical Go idiom for filtered string construction. The implementation is equivalent to the previous version.

Performance — The old approach allocated a []rune(s) up front (full UTF-8 decode) and then re-encoded with string(chars[:n]), always paying the cost of a full rune slice regardless of how many characters are actually removed. strings.Map works byte-by-byte internally and can short-circuit allocation when no runes are removed. The "false" benchmark sub-test (no chars removed) will likely show a measurable improvement for ASCII-heavy strings.

Benchmark — The two sub-tests ("true" = all commas removed, "false" = nothing removed) cover both branches well. Running with -benchmem will confirm the allocation reduction for the no-remove path.

LGTM — clean, idiomatic simplification with a measurable performance benefit for the common no-op case.

@kevwan kevwan added this pull request to the merge queue Mar 22, 2026
Merged via the queue into zeromicro:master with commit 85d770d Mar 22, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants