Skip to content

optimize icall slot allocation and method dispatch - #99

Merged
visualfc merged 3 commits into
goplus:mainfrom
visualfc:ifncached
Aug 5, 2026
Merged

optimize icall slot allocation and method dispatch#99
visualfc merged 3 commits into
goplus:mainfrom
visualfc:ifncached

Conversation

@visualfc

@visualfc visualfc commented Aug 5, 2026

Copy link
Copy Markdown
Member

No description provided.

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 bugmethodof.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 pathmethod_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.

Comment thread methodof.go
ifn, _ = ctx.registerMethod(info, m.FuncId)
if m.FuncId > 0 {
ifn, allocated = ctx.registerMethod(info, m.FuncId)
if m.FuncId > 0 && allocated {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment thread method_test.go
}
}

func BenchmarkReflectCallIndirect(b *testing.B) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@visualfc
visualfc merged commit cbb2727 into goplus:main Aug 5, 2026
18 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.

1 participant