Repository navigation
runtime: optimize slice and string map paths - #3032
Merged
Merged
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review: runtime string-map and slice fast paths
Overall this is a clean, well-reasoned optimization. I verified the two hot-path changes against their surrounding code and the helpers they replace, and ran each dimension (correctness/safety, performance, code quality, docs) over the diff.
What the PR does
map_faststr.go: replacesstringStructOf(&ky)with a stack-localstringStruct{unsafe.StringData(ky), len(ky)}and addshashMapKeyString, which keeps the temporary string header on the stack viac.Allocabefore callingt.Hasher— a faithful mirror of the existinghashMapKey32/hashMapKey64helpers. This removes the per-operation header heap allocation.z_slice.go:NewSlice2/NewSlice3Boundsfold the signed/negative/unsigned index checks into singleuint64(idx) > uint64(bound)comparisons and move theboundsSliceAcap/boundsSliceAlenselection onto the cold panic path.
Correctness & safety — verified sound
- The
uint64rewrite is semantically equivalent to the removedboundsOutOfRange/boundsAbove, givencapis non-negative (confirmed at the call sites inssa/datastruct.go:nCapcomes fromSliceCap/arrayLen/StringLen, all ≥ 0). Negative signed and high-bit unsigned indexes still panic; valid0 ≤ i ≤ j ≤ capstill pass. unsafe.StringDataon an empty key is safe here: thek.len != key.lenguard plusmemequal'ssize == 0short-circuit prevent any zero-size deref, and the 4-byte long-key comparisons run only underkey.len >= 32.- GC-safety is preserved: the
Allocastack header stays live across thet.Hashercall, and the key stored into the bucket roots the same backing bytes (covered byTestMapFastStringKeysSurviveGrowthAndGC). boundsOutOfRange/boundsAboveare correctly retained forz_string.go.
The two inline notes below are minor maintainability suggestions — nothing blocking.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
LLGo WebAssembly build benchmarks
WebAssembly output sizes
LLGo WebAssembly build measurements
Compared with |
LLGo baseline benchmarks
String equality benchmarks
Program measurements
Core language and compiler benchmarks
Timer runtime benchmarks
Compared with |
cpunion
reviewed
Nov 10, 2026
cpunion
left a comment
cpunion
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed 2e2783d. I found no blocking correctness issues.
The string header is borrowed synchronously by the hasher; the empty-key and stored-key paths retain their existing semantics. The slice checks preserve the upper-to-lower ordering: after checking against nonnegative cap, each subsequent bound is known to fit int before conversion. Signedness is still retained for panic formatting, including negative signed and high-bit unsigned indices.
Validation performed with a compiler built from this head:
- Native macOS/arm64: the four new string-map tests at O0/O2, plus the existing map and slice-bounds/panic-text regressions.
- Emscripten32 and Memory64: the same four new string-map tests, copied unchanged into a temporary pure-Go test package to avoid the original package's platform-specific C declarations; Emscripten32 also passed the existing map and bounds regressions.
- Extracted old/new slice checks: 279,000 boundary combinations produced identical results and panic arguments, covering signedness flags and 32/64-bit boundary values. This comparison is not a claim of native 32-bit CPU execution.
One non-blocking comment below clarifies the allocation benefit's platform scope.
cpunion
approved these changes
Nov 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.