icall: optimize

[visualfc]

No description provided.

[fennoai]

Review: icall i_x optimization

No blocking issues found. This applies two well-scoped micro-optimizations to func i_x identically across all 9 non-regabi files (the cmd/icall_gen/_data/icall.go template plus the 8 generated copies), and the template/generator stay in sync so regeneration will not revert the change.

What changed

• cmd/icall_gen/_data/icall.go:88-89,103-105 — preallocate in := make([]reflect.Value, inCount) and index-assign instead of []reflect.Value{receiver} + append. This replaces the 1→2→4… slice regrowth with a single correctly-sized allocation. inCount = NumIn() is always ≥ 1, so in[0] and the 1..inCount-1 loop cover every slot exactly.
• cmd/icall_gen/_data/icall.go:119 — replace the byte-by-byte out-copy loop with copy(unsafe.Slice(...), unsafe.Slice(...)). Both slices are length info.OutSize, so copy moves exactly OutSize bytes (memmove-backed, at least as fast); this block only runs when OutTyp.NumField() > 0.

Verified

• Correctness: both changes are behaviorally equivalent to the prior code.
• Memory safety: po := unsafe.Pointer(out.UnsafeAddr()) is the correct same-expression pattern; po is a real unsafe.Pointer used at the copy, keeping out live. Source read stays within bounds (OutSize ≤ OutTyp.Size()) and destination arithmetic (add(p, info.InSize) + OutSize) is unchanged.
• Consistency: the change is byte-identical across all 9 files at the same lines; the regabi template is correctly left untouched (different code path).

Non-blocking nits (optional)

• _data/icall.go:119 — the single-line copy nests two unsafe.Slice calls plus add(...); extracting dst := add(p, info.InSize, "") would improve readability.
• Out of scope, noted for future follow-up: the per-call buf := make([]byte, sz) (:96) and per-field inArgs.Field(i-1) remain the dominant allocations on the argument side.

[visualfc]

@fennoai

[fennoai]

Review: icall: optimize (PR #97)

I reviewed the PR against the locked range 8d5331d...1739115. This is a solid optimization with one embedded correctness fix. Verified locally on linux/amd64 (Go 1.22.10): go build ./... clean, full root test suite green, and the new TestMethodWrongReturnCount passes.

What the PR does

1. Direct dispatch via MethodInfo.Call. abi.MethodInfo drops Name/FuncId and gains Call func([]reflect.Value) []reflect.Value, set to the user callback m.Func. Both i_x paths now invoke info.Call(in) directly instead of dispatching through the MakeFunc trampoline (info.Func.Call / .CallSlice) and branching on Variadic per call. FuncId becomes an explicit funcID param on registerMethod.
2. Slice preallocation in the non-regabi i_x: make([]reflect.Value, inCount) + indexed assignment replaces append, and the output copy uses copy(unsafe.Slice(...)) instead of a byte loop.
3. //go:nosplit added to regabi i_x and to moveMakeFuncArgPtrs (x/reflect/value.go).
4. arm64 asm fix in icall_regabi_arm64.s.

Findings

✅ Correctness fix — arm64 context argument (the important one). In icall_regabi_arm64.s the macro previously did MOVD R26, 16(RSP) immediately followed by MOVD R3, 16(RSP) (argframe), so the context pointer was written to the frame slot and then clobbered — meaning the first argument c at 8(RSP) was never initialized. i_x(c, frame, retValid, r, index) places args at 8/16/24/32/40(RSP). The change to MOVD R26, 8(RSP) now writes the context into the correct c slot, exactly mirroring the working amd64 layout (MOVQ DX, 0(SP) then argframe → 8(SP)). This is a real bugfix, not just a cleanup. Confirmed the corrected slot is present in all 9 regabi .s copies with zero stale 16(RSP) variants remaining.

✅ Variadic dispatch equivalence. Dropping the Variadic branch in favor of info.Call(in) is safe. info.Call == m.Func is the raw MakeFunc implementation, which expects the variadic argument already packed as a single trailing slice element — which is exactly how i_x builds in from the flattened InTyp fields. The old mfn.CallSlice(in) forwarded that same packed slice to m.Func, so behavior is preserved.

✅ Return-count guard restored + tested. Calling m.Func directly bypasses the return-count validation that reflect.Value.Call used to provide via mfn. The new explicit if len(r) != info.OutTyp.NumField() { panic("reflect: wrong return count from function created by MakeFunc") } restores that safety net, and TestMethodWrongReturnCount covers it. Good call adding the test alongside the behavior change.

✅ //go:nosplit additions. Reasonable for the regabi path: preventing a stack-move/preemption between spillArgs and the write-barrier-free pointer move in moveMakeFuncArgPtrs avoids corrupting register-arg pointers the GC can't track. The nosplit stack-budget check passes at build time (build succeeded with the directives active).

✅ Generated-file consistency. All 8 icall/* sizes + internal/icall512 match the cmd/icall_gen/_data template (differing only by package name and the generated icall_array/icall_fn). No manual drift. Removed symbols (MethodInfo.Name, MethodInfo.FuncId, methodInfoText, methodUsed) have no remaining references.

Minor / non-blocking

• Duplicated panic string. The "reflect: wrong return count from function created by MakeFunc" literal is copy-pasted into every generated icall.go. That's inherent to the codegen model here, so it's fine — just noting it lives in the template (cmd/icall_gen/_data/icall.go), which is the right single source to edit.
• vet warnings are pre-existing. go vet flags unkeyed struct literals in xcall.go:29 and method_test.go:1257-1258 — both unrelated to this PR; not introduced here.
• I could not exercise the arm64 path at runtime (sandbox is linux/amd64). The asm reasoning above is by inspection against the amd64 reference; a CI run on arm64 (or darwin/arm64) is the real confirmation.

Verification

• go build ./... — clean (amd64)
• go test ./ (root suite) — pass
• go test -run TestMethodWrongReturnCount -v ./ — pass
• go test ./x/reflect/... ./icall/... ./internal/... ./abi/... — pass / no test files

Nice cleanup: -104 net lines, removes a per-call branch and a trampoline hop on the hot dispatch path, and fixes a latent arm64 argument-slot bug in the process.