optimize icall slot allocation and method dispatch - #99
Conversation
There was a problem hiding this comment.
Review Summary
This PR does three clean things: fixes globalIfnCached to only count actual slot allocations, drops the dead mlist variable, and replaces the O(capacity) atomic-CAS scan in each icall provider's Insert with an O(1) mutex + free-list. The globalIfnCached counting fix is correct and matches its intent, and the free-list rewrite is internally consistent (len(free) + n == capacity holds; Remove guards its append with Swap(nil) != nil so no double-free; Clear rebuilds via resetFree; allocation order 0,1,2,... is preserved). No critical or newly-introduced correctness bug.
A few findings worth addressing (inline where they land on changed lines):
1. registerMethod uses break where it should continue — latent multi-provider bug — methodof.go:80-82 (pre-existing, unchanged by this PR, so noted here rather than inline)
for i, mp := range abi.Default.List() {
if mp.Available() == 0 { continue }
ifn, mindex := mp.Insert(info)
if mindex == -1 { break } // abandons the whole provider list
...
}Insert now returns -1 when its free-list is empty. With the new mutex, the Available() == 0 fast-path check and the actual Insert are separated, so a provider can fill up between them (or simply report a stale count). On the -1 path the loop breaks, abandoning all remaining providers and incrementing nAllocateError, even though a later provider may have free slots. Harmless with the single default provider, but MethodProviderList explicitly supports multiple providers. Recommend continue instead of break.
2. Stale-ifn slot reuse (pre-existing, but the free-list makes it the common case) — internal/icall*/icall.go i_x / Remove / Clear
i_x loads mp.used[index] locklessly and immediately dereferences it. After Remove/Clear (e.g. Context.Reset), a still-held method value dispatched through its old ifn will either hit a nil *MethodInfo (crash) or, once the free-list re-hands that index to a different method, copy argument/return bytes with mismatched InSize/OutSize (silent corruption). This lifetime hazard existed with the old CAS allocator too, so it is not introduced here — but the O(1) free-list makes immediate reuse of a just-freed slot the normal path, raising the practical likelihood. Consider documenting that calling a method after Reset/Remove of its owning type is undefined, or adding a per-slot generation tag so i_x can detect recycling.
3. Benchmark coverage does not exercise the optimized path — method_test.go
The new/moved BenchmarkReflectCall* benchmarks use reflect.ValueOf(pt).MethodByName(...) on the statically-compiled emtpyCall, so they measure stock reflect dispatch and never touch reflectx's Insert/free-list at all. This PR's actual optimization (registration throughput, O(capacity)→O(1)) is unbenchmarked; a benchmark looping SetMethodSet + ctx.Reset would demonstrate it.
Minor: Available()/Used() read p.n atomically outside the mutex — race-free but best-effort snapshots (benign, pre-existing). Generator templates in cmd/icall_gen/_data/* were verified to carry the free-list change, so regeneration won't revert it, and no stale comments describe the removed scan.
| ifn, _ = ctx.registerMethod(info, m.FuncId) | ||
| if m.FuncId > 0 { | ||
| ifn, allocated = ctx.registerMethod(info, m.FuncId) | ||
| if m.FuncId > 0 && allocated { |
There was a problem hiding this comment.
allocated is declared once at the top of the loop (line 264) and reused here for the value-method registration. It works today because this branch always calls registerMethod before reading allocated, so the pointer-method value is never observed stale. But sharing a single allocated across two independent registrations is fragile — a future edit that reads it before the reassignment would silently double-count globalIfnCached. Consider a fresh local (e.g. var allocated bool inside the if hasIfn block) to make the two sites independent.
| } | ||
| } | ||
|
|
||
| func BenchmarkReflectCallIndirect(b *testing.B) { |
There was a problem hiding this comment.
BenchmarkReflectCallIndirect is identical to BenchmarkReflectCallNoPtr (line 1386): both obtain Info via reflect.ValueOf(...).MethodByName("Info") on the same value receiver and call it the same way. Only the native counterparts differ (value vs pointer receiver). This reflect variant adds no distinct signal — either drop it or make it exercise a genuinely different path.
No description provided.