⚡ Optimize Read loop by replacing expensive modulo arithmetic - #203
⚡ Optimize Read loop by replacing expensive modulo arithmetic#203kpango wants to merge 3 commits into
Read loop by replacing expensive modulo arithmetic#203Conversation
Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
There was a problem hiding this comment.
Pull request overview
This PR aims to micro-optimize (*gache[V]).Read by removing modulo arithmetic from the tight loop that distributes decoded map entries into per-worker chunks.
Changes:
- Replaced
i%numWorkersindexing with a simple counter reset to avoid division in theReadchunking loop. - Added a micro-benchmark file comparing modulo vs branch vs bitwise distribution approaches.
- Added module files (
go.mod,go.sum) and also included large/non-source artifacts (gache.go.orig,patch.diff).
Reviewed changes
Copilot reviewed 4 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| gache.go | Replaces modulo-based chunk selection with a counter reset in Read. |
| benchmark_read_loop3_test.go | Adds benchmarks comparing modulo vs branch vs bitwise indexing. |
| go.mod | Adds module definition and dependency list (appears workflow-generated). |
| go.sum | Adds dependency checksums (appears workflow-generated). |
| gache.go.orig | Adds a full duplicate snapshot of gache.go (artifact). |
| patch.diff | Adds a diff-of-a-diff file (artifact). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| --- gache.go | ||
| +++ gache.go | ||
| @@ -806,8 +806,11 @@ | ||
|
|
||
| i := 0 | ||
| for k, v := range m { | ||
| - chunks[i%numWorkers] = append(chunks[i%numWorkers], kv[V]{key: k, value: v}) | ||
| + chunks[i] = append(chunks[i], kv[V]{key: k, value: v}) | ||
| i++ | ||
| + if i >= numWorkers { | ||
| + i = 0 | ||
| + } | ||
| } | ||
|
|
||
| wg.Add(numWorkers) |
There was a problem hiding this comment.
patch.diff appears to be an artifact (a diff-of-a-diff) rather than a source file used by the library/build. Keeping this file in the repo/PR adds noise and can confuse reviewers/tools; please remove it from the commit.
| --- gache.go | |
| +++ gache.go | |
| @@ -806,8 +806,11 @@ | |
| i := 0 | |
| for k, v := range m { | |
| - chunks[i%numWorkers] = append(chunks[i%numWorkers], kv[V]{key: k, value: v}) | |
| + chunks[i] = append(chunks[i], kv[V]{key: k, value: v}) | |
| i++ | |
| + if i >= numWorkers { | |
| + i = 0 | |
| + } | |
| } | |
| wg.Add(numWorkers) |
| // Package gache provides a high-performance, generic, concurrent-safe in-memory | ||
| // cache library for Go. It uses sharded storage to minimise lock contention and | ||
| // supports automatic expiration, expiration hooks, and serialisation to/from | ||
| // [io.Writer] and [io.Reader]. | ||
| // | ||
| // Basic usage: | ||
| // | ||
| // // Create a new string cache with default settings (30 second TTL). |
There was a problem hiding this comment.
gache.go.orig is a full duplicate snapshot of gache.go (and is not referenced by build tooling). Adding a large copy of the source file increases repo size and maintenance overhead; please drop this file from the PR.
| func BenchmarkReadLoop_Modulo3(b *testing.B) { | ||
| numWorkers := 8 | ||
| m := make(map[string]dummyV, 10000) | ||
| for i := 0; i < 10000; i++ { | ||
| m[fmt.Sprintf("key%d", i)] = dummyV{} | ||
| } | ||
|
|
||
| // Pre-allocate to isolate the loop performance | ||
| chunks := make([][]kv[dummyV], numWorkers) | ||
| for i := range chunks { | ||
| chunks[i] = make([]kv[dummyV], 0, len(m)/numWorkers+1) | ||
| } | ||
|
|
||
| b.ResetTimer() | ||
| for n := 0; n < b.N; n++ { | ||
| for i := range chunks { | ||
| chunks[i] = chunks[i][:0] // reset slices | ||
| } | ||
|
|
||
| i := 0 | ||
| for k, v := range m { | ||
| chunks[i%numWorkers] = append(chunks[i%numWorkers], kv[dummyV]{key: k, value: v}) | ||
| i++ | ||
| } | ||
| } | ||
| } | ||
|
|
||
| func BenchmarkReadLoop_Branch3(b *testing.B) { | ||
| numWorkers := 8 |
There was a problem hiding this comment.
The benchmark file name includes a trailing “3” (e.g., BenchmarkReadLoop_Modulo3) which doesn’t match the rest of the repo’s benchmark naming and makes it harder to discover/compare results. Consider renaming the file/benchmarks to the standard names used elsewhere (or folding these benchmarks into the existing benchmark files) to keep the suite organized.
|
@jules can you show me more about difference? And please remove unneccessary files. |
Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
|
@jules can you improve more about performance? |
I've removed the unnecessary benchmark and diff files from the staging area in the latest commit. Regarding the difference: |
Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
💡 What: Replaced the modulo arithmetic (
i%numWorkers) inside the loop in theReadmethod with an explicit counter reset (if i >= numWorkers { i = 0 }).🎯 Why: Modulo operation (
%) involves division and is computationally expensive on many architectures, especially inside tight loops. Replacing it with a simple branch (if) significantly reduces CPU cycles for loop iteration, particularly when the divisor is not guaranteed to be a power of two (which allows compiler optimization to bitwiseAND).📊 Measured Improvement:
A custom benchmark isolates the array appending loop. Over 10,000 iterations:
chunks[i%numWorkers]: ~173,470 ns/opnumWorkers) depend onGOMAXPROCS, they can't be reliably optimized using bitwiseAND. The branching technique is safer and generally faster than modulo across non-power-of-two inputs without changing application behavior.PR created automatically by Jules for task 6375883000054923944 started by @kpango