purego: remove pooling of syscall#452
Conversation
hajimehoshi
left a comment
hajimehoshi
left a comment
There was a problem hiding this comment.
LGTM
My understanding is, introducing a pool broke some invariations for nosplit functions: is that correct?
There was a problem hiding this comment.
Pull request overview
This PR removes the shared sync.Pool used for *syscallArgs during FFI calls, addressing a concurrency regression where concurrent calls could observe corrupted/swapped return values (issue #451). The change trades reduced allocations for correctness by ensuring each call uses a distinct syscallArgs instance.
Changes:
- Remove
sync.Pool-backed reuse of*syscallArgsand allocate freshsyscallArgsper call path. - Update syscall dispatch implementations (
syscall.go,syscall_32bit.go,syscall_ppc64le.go) to no longerGet/Putpooled structs. - Add a new concurrent malloc/free test intended to exercise the pointer-return path under contention.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| func.go | Removes global sync.Pool and stops pooling *syscallArgs in RegisterFunc call paths. |
| syscall.go | Stops pooling in SyscallN path by allocating a fresh syscallArgs. |
| syscall_32bit.go | Same as above for 32-bit/arm builds. |
| syscall_ppc64le.go | Same as above for ppc64le build path. |
| func_test.go | Adds a new concurrency test invoking malloc/free across goroutines. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var alloc func(uint64) *byte | ||
| var free func(*byte) | ||
| purego.RegisterLibFunc(&alloc, libc, "malloc") | ||
| purego.RegisterLibFunc(&free, libc, "free") | ||
|
|
||
| var wg sync.WaitGroup | ||
|
|
||
| for i := 0; i < runtime.NumCPU(); i++ { | ||
| wg.Add(1) | ||
| go func(id int) { | ||
| defer wg.Done() | ||
| for j := 0; j < 400_000; j++ { | ||
| ptr := alloc(5) | ||
| if ptr == nil { | ||
| continue | ||
| } | ||
| free(ptr) | ||
| } |
| var wg sync.WaitGroup | ||
|
|
||
| for i := 0; i < runtime.NumCPU(); i++ { | ||
| wg.Add(1) | ||
| go func(id int) { | ||
| defer wg.Done() | ||
| for j := 0; j < 400_000; j++ { |
|
Still LGTM (it's up to you to address on the Codex suggestions) |
|
I don't think it's necessary to make those changes |
|
Sure. Let's merge this then. |
|
@hajimehoshi still waiting on approval |
hajimehoshi
left a comment
hajimehoshi
left a comment
There was a problem hiding this comment.
LGTM (Sorry I think I have already approved)
What issue is this addressing?
Closes #451
What type of issue is this addressing?
Bug
What this PR does | solves
This removes the pool which was occasionally causing issues. Unfortunately this does increase the memory cost of each call