icall: replace map with slice#76
Conversation
![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 updates the project to Go 1.21 and refactors the provider struct across multiple packages to use slices instead of maps for managing method information. This change includes the introduction of a usage counter n and the adoption of the clear built-in. Review feedback identifies two significant issues: first, the Remove method lacks idempotency and incorrectly decrements the counter even if a slot is already empty; second, in regabi implementations, the Insert method increments the counter even when no available slot is found (index is -1), which could lead to inconsistent state or panics.
| for _, n := range indexs { | ||
| delete(p.used, n) | ||
| p.used[n] = nil | ||
| p.n-- | ||
| } |
There was a problem hiding this comment.
The current implementation of Remove decrements the counter p.n for every index in indexs, even if the slot was already empty (nil). This can lead to an incorrect count if duplicate indices are provided or if an index is removed twice. The previous map-based implementation was idempotent because delete on a map is a no-op if the key is missing. Consider checking if the slot is non-nil before decrementing.
for _, n := range indexs {
if p.used[n] != nil {
p.used[n] = nil
p.n--
}
}| for _, n := range indexs { | ||
| delete(p.used, n) | ||
| p.used[n] = nil | ||
| p.n-- | ||
| } |
There was a problem hiding this comment.
The current implementation of Remove decrements the counter p.n for every index in indexs, even if the slot was already empty (nil). This can lead to an incorrect count if duplicate indices are provided or if an index is removed twice. The previous map-based implementation was idempotent because delete on a map is a no-op if the key is missing. Consider checking if the slot is non-nil before decrementing.
for _, n := range indexs {
if p.used[n] != nil {
p.used[n] = nil
p.n--
}
}| for _, n := range indexs { | ||
| delete(p.used, n) | ||
| p.used[n] = nil | ||
| p.n-- | ||
| } |
There was a problem hiding this comment.
The current implementation of Remove decrements the counter p.n for every index in indexs, even if the slot was already empty (nil). This can lead to an incorrect count if duplicate indices are provided or if an index is removed twice. The previous map-based implementation was idempotent because delete on a map is a no-op if the key is missing. Consider checking if the slot is non-nil before decrementing.
for _, n := range indexs {
if p.used[n] != nil {
p.used[n] = nil
p.n--
}
}| for _, n := range indexs { | ||
| delete(p.used, n) | ||
| p.used[n] = nil | ||
| p.n-- | ||
| } |
There was a problem hiding this comment.
The current implementation of Remove decrements the counter p.n for every index in indexs, even if the slot was already empty (nil). This can lead to an incorrect count if duplicate indices are provided or if an index is removed twice. The previous map-based implementation was idempotent because delete on a map is a no-op if the key is missing. Consider checking if the slot is non-nil before decrementing.
for _, n := range indexs {
if p.used[n] != nil {
p.used[n] = nil
p.n--
}
}| for _, n := range indexs { | ||
| delete(p.used, n) | ||
| p.used[n] = nil | ||
| p.n-- | ||
| } |
There was a problem hiding this comment.
The current implementation of Remove decrements the counter p.n for every index in indexs, even if the slot was already empty (nil). This can lead to an incorrect count if duplicate indices are provided or if an index is removed twice. The previous map-based implementation was idempotent because delete on a map is a no-op if the key is missing. Consider checking if the slot is non-nil before decrementing.
for _, n := range indexs {
if p.used[n] != nil {
p.used[n] = nil
p.n--
}
}| fun: fn, | ||
| ptr: (*struct{ typ, ptr unsafe.Pointer })(unsafe.Pointer(&fn)).ptr, | ||
| } | ||
| p.n++ |
There was a problem hiding this comment.
The counter p.n is incremented here regardless of whether a valid slot was found in the initial loop. If index is -1, this increment is incorrect (though the subsequent code will likely panic). Consider incrementing p.n only when a slot is successfully allocated, similar to the implementation in icall.go.
| fun: fn, | ||
| ptr: (*struct{ typ, ptr unsafe.Pointer })(unsafe.Pointer(&fn)).ptr, | ||
| } | ||
| p.n++ |
There was a problem hiding this comment.
The counter p.n is incremented here regardless of whether a valid slot was found in the initial loop. If index is -1, this increment is incorrect (though the subsequent code will likely panic). Consider incrementing p.n only when a slot is successfully allocated, similar to the implementation in icall.go.
| fun: fn, | ||
| ptr: (*struct{ typ, ptr unsafe.Pointer })(unsafe.Pointer(&fn)).ptr, | ||
| } | ||
| p.n++ |
There was a problem hiding this comment.
The counter p.n is incremented here regardless of whether a valid slot was found in the initial loop. If index is -1, this increment is incorrect (though the subsequent code will likely panic). Consider incrementing p.n only when a slot is successfully allocated, similar to the implementation in icall.go.
| fun: fn, | ||
| ptr: (*struct{ typ, ptr unsafe.Pointer })(unsafe.Pointer(&fn)).ptr, | ||
| } | ||
| p.n++ |
There was a problem hiding this comment.
The counter p.n is incremented here regardless of whether a valid slot was found in the initial loop. If index is -1, this increment is incorrect (though the subsequent code will likely panic). Consider incrementing p.n only when a slot is successfully allocated, similar to the implementation in icall.go.
| fun: fn, | ||
| ptr: (*struct{ typ, ptr unsafe.Pointer })(unsafe.Pointer(&fn)).ptr, | ||
| } | ||
| p.n++ |
There was a problem hiding this comment.
The counter p.n is incremented here regardless of whether a valid slot was found in the initial loop. If index is -1, this increment is incorrect (though the subsequent code will likely panic). Consider incrementing p.n only when a slot is successfully allocated, similar to the implementation in icall.go.
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review: icall: replace map with slice
The overall direction — replacing map[int]*T with a pre-allocated []*T slice and an n counter — is sound and improves cache locality and removes map overhead for the common case. A few issues across the generated files are worth addressing.
Summary of findings:
- CI matrix not fully updated (Linux/Windows still test pre-Go-1.21 versions → compile failure)
Removedoes not guard against double-remove →p.ncounter underflowRemovehas no bounds check → runtime panic on OOB index (regression from map which was a silent no-op)InsertO(capacity) linear scan from index 0 with no free-slot hint; notable at large capacities- Large providers pre-allocate full capacity upfront (e.g.
icall20480= 160 KB atinit) clear(p.used)vsclear(p.used[:])inconsistency between template files- README ABI section still documents Go 1.18/1.19 behaviour below the new minimum
| strategy: | ||
| matrix: | ||
| go-version: [1.18.x, 1.19.x, 1.20.x, 1.21.x, 1.22.x, 1.23.x, 1.24.x, 1.25.x, 1.26.x] | ||
| go-version: [1.21.x, 1.22.x, 1.23.x, 1.24.x, 1.25.x, 1.26.x] |
There was a problem hiding this comment.
CI breakage: Linux matrix not updated. This job still includes 1.18.x, 1.19.x, 1.20.x, but go.mod now requires Go 1.21 and clear() (added in 1.21) is used throughout — these versions will fail at compile time. The MacOS matrix was updated in this PR but Linux (here) and Windows (line ~54) were not. Both should be updated to match: [1.21.x, 1.22.x, 1.23.x, 1.24.x, 1.25.x, 1.26.x].
| strategy: | ||
| matrix: | ||
| go-version: [1.18.x, 1.19.x, 1.20.x, 1.21.x, 1.22.x, 1.23.x, 1.24.x, 1.25.x, 1.26.x] | ||
| go-version: [1.21.x, 1.22.x, 1.23.x, 1.24.x, 1.25.x, 1.26.x] |
There was a problem hiding this comment.
CI breakage: Windows matrix not updated. Same issue as the Linux job above — still includes Go 1.18/1.19/1.20 which will fail to compile this module.
| func (p *provider) Remove(indexs []int) { | ||
| for _, n := range indexs { | ||
| delete(p.used, n) | ||
| p.used[n] = nil | ||
| p.n-- |
There was a problem hiding this comment.
Remove: two correctness issues introduced by the map→slice change (applies to all generated files).
-
No nil-guard → counter underflow.
p.n--runs unconditionally even ifp.used[n]is alreadynil(double-remove). This makesp.ngo negative, causingAvailable()to exceedcapacityandUsed()to return a negative value. The map version was implicitly safe (deleteon a missing key was a no-op). -
No bounds check → runtime panic. An
nvalue outside[0, capacity)panics withindex out of range. The map version was a silent no-op for any key.
Suggested fix:
func (p *provider) Remove(indexs []int) {
for _, n := range indexs {
if n >= 0 && n < capacity && p.used[n] != nil {
p.used[n] = nil
p.n--
}
}
}This fix needs to be applied in the template (cmd/icall_gen/_data/icall.go) and regenerated, or applied uniformly across all generated files.
| func (p *provider) Remove(indexs []int) { | ||
| for _, n := range indexs { | ||
| delete(p.used, n) | ||
| p.used[n] = nil | ||
| p.n-- | ||
| } |
There was a problem hiding this comment.
Same Remove issues as in icall.go — no nil-guard and no bounds check. Both need the same fix in this template file.
| func (p *provider) Insert(info *abi.MethodInfo) (ifn unsafe.Pointer, index int) { | ||
| for i := 0; i < capacity; i++ { | ||
| if _, ok := p.used[i]; !ok { | ||
| if p.used[i] == nil { | ||
| p.n++ | ||
| p.used[i] = info | ||
| fn := icall_array[i] | ||
| return unsafe.Pointer(reflect.ValueOf(fn).Pointer()), i |
There was a problem hiding this comment.
Insert is O(capacity) with no free-slot hint. Every call scans from index 0, so if the first free slot is near the end (e.g. after many removes), performance degrades to O(capacity). For the 20480-slot provider this is up to 20480 pointer comparisons per insertion. A single nextFree int field that advances past occupied slots (with wrap-around) would give O(1) amortized Insert without complicating the rest of the implementation.
| mp = &provider{ | ||
| used: make(map[int]*abi.MethodInfo), | ||
| used: make([]*abi.MethodInfo, capacity), | ||
| } | ||
| ) | ||
|
|
There was a problem hiding this comment.
Full-capacity pre-allocation at init time. make([]*abi.MethodInfo, 20480) commits ~160 KB of heap immediately at program startup, regardless of how many methods are actually registered. The old make(map[int]*T) was near-zero until populated. Programs that import large-capacity providers but register few methods now pay this cost unconditionally. Consider lazy initialization or a smaller initial allocation.
|
|
||
| func (p *provider) Clear() { | ||
| p.used = make(map[int]*abi.MethodInfo) | ||
| clear(p.used) |
There was a problem hiding this comment.
clear(p.used) vs clear(p.used[:]) inconsistency. icall.go template uses clear(p.used) while icall_regabi.go uses clear(p.used[:]). Both are semantically identical for a slice, but the inconsistency suggests independent edits rather than uniform code generation. Standardising to one form in the templates avoids confusion.
| ### Go Version | ||
|
|
||
| - Go1.18 ~ Go1.26 | ||
| - Go1.21 ~ Go1.26 |
There was a problem hiding this comment.
The top-level Go version line is correctly updated, but the ABI table a few lines below still documents per-platform behaviour starting at Go 1.18 and Go 1.19 — both below the new minimum. Those rows will never apply to users of this module now. Consider removing or collapsing them.
PR #76 Review: icall: replace map with sliceThis PR makes two independent sets of changes:
Issues Found1. In if n < capacity && p.used[n] != nil {A negative index passed by a buggy/malicious caller would still panic with 2. In 3. CI workflow: duplicate
4.
5.
6. Build tag comment inconsistency in The //go:build ... && go1.23 && !linknamefix
// +build goexperiment.regabiargs amd64 arm64 ppc64 ppc64le riscv64 go1.23,loong64
// +build go1.23
// +build !linknamefixThe Positive aspects
|
visualfc
force-pushed
the
icall_slice
branch
2 times, most recently
from
c0a2665 to
cdf7fd8
Compare
PR #76 Review (updated): icall: replace map with sliceThe PR has been updated since the previous review. The CI matrix issues (all three jobs — MacOS, Linux, Windows — now correctly drop Go 1.18/1.19/1.20) and the 1. In all files ( if n < capacity && p.used[n] != nil {A negative index would bypass the upper-bound check and panic with 2.
3. Build tag legacy comment incomplete in //go:build (goexperiment.regabiargs || amd64 || arm64 || ppc64 || ppc64le || riscv64 || (go1.23 && loong64)) && go1.23 && !linknamefix
// +build goexperiment.regabiargs amd64 arm64 ppc64 ppc64le riscv64 go1.23,loong64
// +build go1.23
// +build !linknamefixThe 4.
|
1 participant
No description provided.