Improve performance - #205
Merged
Merged
Conversation
Also try to convert directly to generic_args pointer
jhawthorn
marked this pull request as ready for review
July 13, 2026 17:34
jeremy
added a commit
to jeremy/fiddle
that referenced
this pull request
Aug 12, 2026
A TYPE_CONST_STRING argument that comes from a to_str object is coerced inside Fiddle, so the caller's argv holds that object and not the String that the char * points into. Only converted_args holds the coerced String, and the conservative scan of the ALLOCV buffer is what keeps it in place while ffi_call runs without the GVL. Nothing tested that. Before ruby#205, converted_args was a Ruby Array, whose elements the GC may move, and the C function read the slot the String left. ruby#205 replaced the Array with this buffer and corrected the fault as a side effect. A later change could move these values back to storage the GC may relocate, and no test would notice. Add the missing test, and record the requirement next to the buffer.
kou
pushed a commit
that referenced
this pull request
Aug 13, 2026
## Problem
`TYPE_CONST_STRING` stores a pointer into a Ruby String:
```c
case TYPE_CONST_STRING:
...
else { dst->pointer = rb_string_value_cstr(src); }
```
A String argument is safe. `argv` holds it and keeps it in place.
A `to_str` object is not. `rb_string_value_cstr` writes the coerced
String back into `src`. `argv` still holds the original object, not the
String. The next loop turn overwrites `src`. After that, only
`converted_args` holds the String.
The pointer must stay valid until `ffi_call` returns. `ffi_call` runs
under `rb_thread_call_without_gvl`, so another thread can compact the
heap during the call. `converted_args` must keep the String **in
place**, not only alive.
Master does this. `converted_args` sits in the `ALLOCV` buffer. The GC
scans that buffer conservatively, and a conservative scan pins the
object.
## Gap
No test covers this.
Before #205, `converted_args` was a Ruby Array. The GC may move an
Array's elements, so the String moved while the pointer stayed, and the
C function read the vacated slot. #205 replaced the Array with the
buffer and fixed the fault as a side effect. Nothing records that the
buffer is required.
## Change
1. `test_call_const_string_from_to_str_after_compaction` in
`test/fiddle/test_function.rb`.
2. A comment at `converted_args` in `ext/fiddle/function.c` stating the
requirement.
The test calls `strcspn` as `[TYPE_CONST_STRING, TYPE_VOIDP] ->
TYPE_SIZE_T`. Arg 0 is a `to_str` object returning a fresh 100-byte
String. Arg 1 is a `to_ptr` object, which makes Fiddle call
`Pointer.[]`; that allocation lets the GC run after the pointer is
stored.
The subject holds no `"Z"`; the reject set is `"Z"`. A correct call
returns 100. A stale pointer returns 0, because a vacated slot holds
zero bytes.
Three details are load-bearing. Remove any one and the test passes on a
faulty build; each has a comment:
- Churn is the subject's byte size, half of it dropped. The compactor
only evacuates non-full pages.
- `to_ptr` returns a **new** `Pointer` each time. An existing `Pointer`
gives Fiddle nothing to allocate.
- The intact-case call runs **after** the loop. A call before it
prepares the CIF, and that preparation is part of what opens the window.
## Result
| tree | arm64-darwin, ruby 3.4.10 | aarch64-linux, ruby 3.4.7 |
|---|---|---|
| `d389bbf` (before #205) | fail | fail |
| master | pass | pass |
Three runs per tree on darwin, same result each time. Full suite on
master with this change: 242 tests, 668 assertions, 0 failures, 1 error,
3 omissions. The error is `test_nogvl_poll` needing `envutil`; it fails
the same way without this change.
Environment: ruby 3.4.10 [arm64-darwin23]; ruby 3.4.7 [aarch64-linux],
glibc 2.36, libffi 3.4.4.
## Note
Released versions still have the fault. I measured 1.1.6 and 1.1.8 on
both platforms: a two-argument call of this shape returns the wrong
result on every iteration under `GC.stress` with `GC.auto_compact`.
Master is correct. You may want a release.
I report this as latent. The trigger is the declared argument type plus
a `to_str` object in place of a String. In one application that loads
fiddle at boot, all 227 bundled gems put every `Fiddle::Function.new`
behind a Windows-only branch.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.