Repository navigation
Conversation
hajimehoshi
left a comment
hajimehoshi
left a comment
There was a problem hiding this comment.
Posted by Claude (Claude Code) on behalf of hajimehoshi. All of these are suggestions, and none is a confirmed failure. The PR's code was not built or run for this review; the findings come from reading the code and the CI results.
|
Can you split this change into two PRs? 1) optimizing callbacks, and 2) the ARM trampoline fix. Thanks, |
Sure, I can split them. I included the ARM fix because repeated callback registration during testing exposed a pre-existing ARMv5 trampoline bug. The assembler can insert literal pools when loading larger callback indices, breaking the assumed fixed entry size and causing an illegal instruction. Without that fix, the callback tests failed. I'll submit the ARM fix with its regression test as a separate PR, then rebase the callback optimization once that fix is merged. |
Select typed dispatch once when registering callbacks with zero through nine uintptr arguments and a uintptr result. Read arguments from the existing register and stack frames without casting Go function types or changing the assembly trampolines. Preserve reflection dispatch for all other signatures and the existing Windows runtime implementation. Add native C callback tests for full-width values, register and stack arguments, foreign threads, GC, concurrent and nested calls, fallback signatures, and allocation growth. Add a batched C-to-Go callback benchmark so registration and outgoing FFI costs do not dominate measurements.
Store one immutable invoker per callback in an atomic slot and wrap reflection callbacks at registration. Read word arguments through an inlinable accessor and share integer-register addressing with reflection dispatch. Remove the unused intFrame hooks, whose implementations all returned nil.
Require zero allocation growth across native callbacks, and reuse callback slots across repeated tests. Exercise concurrent publication and measure parallel callback throughput. Move the pthread fixture into its own library and prebuild it separately for Android.
ab71280 to
21c6c5e
Compare
syscall_unix.go builds on illumos since illumos/amd64 support was added, but callback_word.go did not, so compileWordCallback was undefined there. Use the same build constraint as syscall_unix.go, and run the word callback tests on illumos like the other callback tests.
Asutorufa
changed the title
|
Rebased onto main now that #563 is merged. The ARM trampoline commit is dropped, so this PR now contains only the callback optimization:
The changes to the old I updated the title and description to match. CI passes: https://github.com/ebitengine/purego/actions/runs/42354983651 |
Put each field of the fallback callback test cases on its own line, and expand the function literals whose bodies are statements, so they read like the rest of the code.
hajimehoshi
left a comment
hajimehoshi
left a comment
There was a problem hiding this comment.
LGTM
@TotallyGamerJet PTAL
|
Do we want to do this? My thoughts are
|
Which part are you talking about? The existing files are not changed so much. |
What issue is this addressing?
Unix
NewCallbackcurrently constructs reflection arguments and callsreflect.Value.Callon every invocation. For callbacks called repeatedly from C,this introduces allocation and dispatch costs even when the Go signature consists
entirely of machine words.
What type of issue is this addressing?
Performance.
What this PR does | solves
Select a typed invoker once when registering an unnamed Go function with zero
through nine
uintptrarguments and a singleuintptrresult. Such callbacksread full-width integer slots from the existing saved C argument frame and call
the original Go function directly through an inlinable word accessor. No Go
function type is reinterpreted, and no argument scratch array is needed.
Publish one immutable invoker per callback through an atomic pointer. Slot
allocation remains serialized at registration; dispatch only loads and invokes
the published function. Reflection callbacks are wrapped at registration so the
hot path needs neither a mutex nor a path-selection branch. Both dispatch paths
share integer-register addressing, and the unused
intFramehooks are removed.The existing reflection path handles every other signature, including named
function/argument types, pointers, floating-point values, narrow integers,
CDecl, void results, and larger arities. Windows continues to use the runtime'scallback implementation. Public API behavior and callback slot limits remain
unchanged.
Add native C-to-Go tests for all specialized arities, high-bit argument/return
values, register/stack boundaries, allocation growth, foreign pthreads, GC,
concurrent registration/nested callbacks, and fallback signatures. Allocation
coverage compares 0 and 64 iterations and requires no growth. Test callbacks are
cached for the process lifetime so repeated runs do not consume more slots.
The new C helpers live in a separate pthread-linked fixture, also prebuilt for
Android; existing fixture sources and builder flags remain unchanged. Batched
serial and parallel benchmarks amortize outgoing FFI costs over 256 callbacks.
Benchmarks
Linux amd64, AMD Ryzen 5 5600G, Go 1.27.3, CGO_ENABLED=0,
GOMAXPROCS=1, pinned to CPU 2; medians of six 200ms samples. Baseline is
ebitengine/puregomain at7fd5298cee0ac33e76d0e40c2ce6fcf066d6a2fa,with only the new C fixture and benchmark copied into that checkout.
ns/callback = elapsed ns / (benchmark operations * 256). The table takesmedians of the reported
ns/callbacksamples, then rounds to two decimal placesusing decimal rounding (ties up). Time reduction is
(before median - after median) / before median * 100%, calculated before rounding.The parallel table below uses the same median and rounding rules.
Each benchmark operation invokes 256 callbacks from one C loop. Allocations per
operation fall from 517 (0/1 arguments) or 773 (2–9 arguments) to 5. The remaining
5 allocations belong to the one outgoing RegisterFunc call, rather than each
C-to-Go callback. The callback body itself performs no heap allocation.
Existing int64 callback benchmarks remain on reflection, with unchanged
allocation counts. Median time changes in the same six-sample setup were -2.7%
(1 argument), -0.5% (2), +4.7% (3), -0.6% (5), -1.0% (10), +0.5% (14), and
+1.5% (15). These local samples do not establish cross-platform timings.
The parallel benchmark below compares an earlier revision of this PR,
03ef423,with the revised implementation using the same new test fixture and benchmark in both checkouts.
Same host and Go version; six 200ms samples, pinned to logical CPUs 2–9.
Numbers express aggregate throughput across workers, not individual latency.
Validation
This branch is rebased onto main after #563, which fixed the ARM trampoline
spacing separately. The word callback code now builds on illumos as well, with
the same build constraint as
syscall_unix.go.On the rebased head:
with CGO and the race detector enabled (three shuffled repetitions of the root
package).
three shuffled repetitions each, built and run the same way as CI.
go vetfor both CGO configurations, and with CGO disabled for every targetCI cross-compiles: Linux 386/amd64/arm/arm64/loong64/ppc64le/riscv64/s390x,
Darwin amd64/arm64, FreeBSD amd64/arm64, NetBSD amd64/arm64, illumos amd64, and
Windows 386/amd64/arm64. Cross-compilation alone does not validate calling
conventions.
On the earlier revision, before the rebase:
disabled and two repetitions with race/cgocheck2 enabled.
allocations.
-Wl,-z,defs, and the new callback tests executedthrough
PUREGO_TEST_PREBUILT_LIBDIR.