Protect FFI::Function with Write Barrier #991

casperisfine · 2023-02-07T11:22:23Z

Write barrier protected objects are allowed to be promoted to the old generation, which means they only get marked on major GC.

The downside is that the RB_BJ_WRITE macro MUST be used to set references, otherwise the referenced object may be garbaged collected.

cc @peterzhu2118

NB: The test suite use an X86_64 fixtures, so I can't run it locally. I did check that the extension does compile though.

casperisfine · 2023-02-07T11:42:15Z

Hum, look like I missed one of the Get_Struct but it's hard to identify it without being able to run the test suite locally :/

LoadError:
  Could not open library '/Users/byroot/src/github.com/casperisfine/ffi/spec/ffi/fixtures/libtest.dylib': dlopen(/Users/byroot/src/github.com/casperisfine/ffi/spec/ffi/fixtures/libtest.dylib, 0x0005): tried: '/Users/byroot/src/github.com/casperisfine/ffi/spec/ffi/fixtures/libtest.dylib' (mach-o file, but is an incompatible architecture (have 'x86_64', need 'arm64')), '/System/Volumes/Preboot/Cryptexes/OS/Users/byroot/src/github.com/casperisfine/ffi/spec/ffi/fixtures/libtest.dylib' (no such file), '/Users/byroot/src/github.com/casperisfine/ffi/spec/ffi/fixtures/libtest.dylib' (mach-o file, but is an incompatible architecture (have 'x86_64', need 'arm64')).
  Searched in <system library path>, /usr/lib, /usr/local/lib, /opt/local/lib, /opt/homebrew/lib

ext/ffi_c/Function.c

casperisfine · 2023-02-22T10:46:54Z

Ok, I figured why I couldn't run tests locally. I had some old ARCHFLAGS in my bashrc.

Now I also located the actual error.

Function "inherits" from Pointer:

typedef struct Function_ {
    Pointer base;
    FunctionType* info;

And is sometimes passed to ptr_equals:

static VALUE
ptr_equals(VALUE self, VALUE other)
{
    Pointer* ptr;

    Data_Get_Struct(self, Pointer, ptr);

    if (NIL_P(other)) {
        return ptr->memory.address == NULL ? Qtrue : Qfalse;
    }

    return ptr->memory.address == POINTER(other)->address ? Qtrue : Qfalse;
}

So there's some kind of polymorphism going on, which I need to find a solution to preserve.

casperisfine · 2023-02-22T12:22:00Z

Ok, I found a solution, I think the result isn't too bad.

I just refactored Pointer.c and AbstractMemory.c to always go through the macro helpers, and then modified rbffi_AbstractMemory_Cast to check for TypedData.

@larskanis I know it's quite a big change, but is this something you'd be interested on?

The reason we're interested in this is that we're trying to improve GC performance and we noticed we have over a thousand instance of FFI objects that have to be marked on every minors even though they never change:

 ["All FFI::* Types combined", 1303],

 ["DATA(FFI::FunctionType)", 336],
 ["DATA(FFI::Function)", 322],
 ["DATA(FFI::DynamicLibrary::Symbol)", 321],
 ["DATA(FFI::StructLayout::Number)", 76],
 ["DATA(FFI::StructLayout)", 41],
 ["DATA(FFI::DynamicLibrary)", 31],
 ["DATA(FFI::Type::Mapped)", 29],
 ["DATA(FFI::StructLayout::Pointer)", 27],
 ["DATA(FFI::Type::Builtin)", 21],
 ["DATA(FFI::StructByValue)", 18],
 ["DATA(FFI::StructLayout::Mapped)", 17],
 ["DATA(FFI::StructLayout::InnerStruct)", 17],
 ["DATA(FFI::StructLayout::String)", 13],
 ["DATA(FFI::ArrayType)", 12],
 ["DATA(FFI::StructLayout::Array)", 12],
 ["DATA(FFI::StructLayout::Function)", 10],

larskanis · 2023-02-23T18:13:08Z

Sorry Jean for not responding earlier! Sure I'm interested.

Nevertheless I think this can be solved better by converting all derivations of FFI::AbstractMemory to TypedData and make use of the parent element of the rb_data_type_t to describe the inheritance. Here is how I did it in ruby-pg where inheritance is implemented very similar: ged/ruby-pg@a5fbe31

TypedData_Get_Struct is meant to be used with a static pointer to the rb_data_type_t of the base class or a derivation depending of how much of the derived C struct you need to access.

casperisfine · 2023-02-23T18:18:15Z

Sorry Jean for not responding earlier!

No worries, the PR was not passing a single test yet, I wasn't expecting any response at that stage :)

solved better by converting all derivations of FFI::AbstractMemory to TypedData

Right, I was trying to do bite sized changes over multiple PRs to make it easier. But if you are down to migrate all the derivatives of AbstractMemory in one go, I'm happy to oblige.

Here is how I did it in ruby-pg where inheritance is implemented very similar:

Ah I see, you use RTYPEDDATA_DATA(VALUE) to bypass the type check, that make sense, thank you I can use that to simplify.

I'll try to find some time to convert more types soon, I'll let you know how it goes.

Thanks for the pointers!

larskanis · 2023-02-23T19:01:10Z

Write barrier protected objects are allowed to be promoted to the old generation, which means they only get marked on major GC. The downside is that the RB_BJ_WRITE macro MUST be used to set references, otherwise the referenced object may be garbaged collected.

Thank you for this concise description of write-barriers in ruby! I never understood the description in https://github.com/ruby/ruby/blob/master/doc/extension.rdoc#write-barriers- , so that I think it should be added or replaced there.

casperisfine · 2023-02-24T13:49:52Z

Ok, so there wasn't as many as I feared, I converted all the derivations of AbstractMemory.

However it seems like I introduced a GC bug that I'm having a hard time tracking down. It doesn't happen on all test suite runs:

[BUG] Segmentation fault at 0x0000000000000110

-- C level backtrace information -------------------------------------------
/opt/rubies/3.2.1/lib/libruby.3.2.dylib(rb_vm_bugreport+0x9a0) [0x10361ba98]
/opt/rubies/3.2.1/lib/libruby.3.2.dylib(rb_bug_for_fatal_signal+0x160) [0x10343fe14]
/opt/rubies/3.2.1/lib/libruby.3.2.dylib(sig_do_nothing+0x0) [0x10357a354]
/usr/lib/system/libsystem_platform.dylib(_sigtramp+0x38) [0x1806702a4]
/opt/rubies/3.2.1/lib/libruby.3.2.dylib(gc_mark_ptr+0x24) [0x1034712a8]
/opt/rubies/3.2.1/lib/libruby.3.2.dylib(gc_marks_rest+0x9c) [0x10346a050]
/opt/rubies/3.2.1/lib/libruby.3.2.dylib(gc_start+0xd9c) [0x1034705b0]
/opt/rubies/3.2.1/lib/libruby.3.2.dylib(newobj_alloc+0xc4) [0x103469a3c]
/opt/rubies/3.2.1/lib/libruby.3.2.dylib(rb_wb_protected_newobj_of+0xd0) [0x10345bcb8]
/opt/rubies/3.2.1/lib/libruby.3.2.dylib(rb_str_resurrect+0xe4) [0x10358c9bc]
/opt/rubies/3.2.1/lib/libruby.3.2.dylib(rb_str_concat_literals+0xc8) [0x103590acc]

I'll see if Peter can help me figure that one out.

casperisfine · 2023-02-24T13:55:45Z

Nevermind I found a way to identify the bug. It's from buffer_mark. With the old API some buffer were created without a mark function and some with. I need to better port this to the new API.

Write barrier protected objects are allowed to be promoted to the old generation, which means they only get marked on major GC. The downside is that the `RB_BJ_WRITE` macro MUST be used to set references, otherwise the referenced object may be garbaged collected.

casperisfine · 2023-02-24T14:03:08Z

Ok, that should be fixed, I had to check for ptr->memory.flags & MEM_EMBED in buffer_mark to avoid calling rb_gc_mark(ptr->data.rbParent) on standalone buffers.

larskanis · 2023-03-01T08:10:14Z

@casperisfine One spec failed with a segfault in rb_gc_call_finalizer_at_exit . This didn't happen before this PR. Will you double check this?

casperisfine · 2023-03-01T08:25:11Z

Looking.

casperisfine · 2023-03-01T16:42:43Z

Hum, I tried to figure it out, but the failure being on windows with little information, I can't figure it out.

I think this PR grew too much in scope, which makes it hard to debug.

If that's OK with you I think I'll try to do this again but more incrementally. I'll try to do small focused PRs to migrate types one by one to TypedData, and then later try to add write barriers.

Ref: ffi#991 The old untyped DATA API is soft deprecated and this new one open the door to write barriers, compaction, memsize etc.

Ref: ffi#991 The old untyped DATA API is soft deprecated 8000 and this new one open the door to write barriers, compaction, memsize etc.

casperisfine · 2023-03-06T09:33:39Z

Closing this as it derived way too much now that we migrated all types to TypedData. I'll open another one that is only changing Function.

Ref: ffi#991 Write barrier protected objects are allowed to be promoted to the old generation, which means they only get marked on major GC. The downside is that the RB_BJ_WRITE macro MUST be used to set references, otherwise the referenced object may be garbaged collected. This commit also implement a `dsize` function so that these instance report a more relevant size in various memory profilers. It's not counting everything because some types are opaque right now, so a larger refactoring would be needed.

And FFI::DynamicLibrary::Symbol Ref: ffi#991 Write barrier protected objects are allowed to be promoted to the old generation, which means they only get marked on major GC. The downside is that the RB_BJ_WRITE macro MUST be used to set references, otherwise the referenced object may be garbaged collected. This commit also implement a `dsize` function so that these instance report a more relevant size in various memory profilers. It's not counting everything because some types are opaque right now, so a larger refactoring would be needed. While I was at it, I moved `Symbol.library` into the unused `Symbol.base.rbParent` which seemed appropriate and saves a pointer.

Ref: ffi#991 Write barrier protected objects are allowed to be promoted to the old generation, which means they only get marked on major GC. The downside is that the RB_BJ_WRITE macro MUST be used to set references, otherwise the referenced object may be garbaged collected. This commit also implement a `dsize` function so that these instance report a more relevant size in various memory profilers. While I was at it I removed the `strdup` of `BuiltinType.name`, I don't see why we wouldn't directly point at static memory, that saves copying type names, and makes freeing builtin types easier.

Ref: ffi#991 Write barrier protected objects are allowed to be promoted to the old generation, which means they only get marked on major GC. The downside is that the RB_BJ_WRITE macro MUST be used to set references, otherwise the referenced object may be garbaged collected. This commit also implement a `dsize` function so that these instance report a more relevant size in various memory profilers. It's not counting everything because some types are opaque right now, so a larger refactoring would be needed.

And FFI::DynamicLibrary::Symbol Ref: ffi#991 Write barrier protected objects are allowed to be promoted to the old generation, which means they only get marked on major GC. The downside is that the RB_BJ_WRITE macro MUST be used to set references, otherwise the referenced object may be garbaged collected. This commit also implement a `dsize` function so that these instance report a more relevant size in various memory profilers. It's not counting everything because some types are opaque right now, so a larger refactoring would be needed. While I was at it, I moved `Symbol.library` into the unused `Symbol.base.rbParent` which seemed appropriate and saves a pointer.

Ref: ffi#991 Write barrier protected objects are allowed to be promoted to the old generation, which means they only get marked on major GC. The downside is that the RB_BJ_WRITE macro MUST be used to set references, otherwise the referenced object may be garbaged collected. This commit also implement a `dsize` function so that these instance report a more relevant size in various memory profilers. While I was at it I removed the `strdup` of `BuiltinType.name`, I don't see why we wouldn't directly point at static memory, that saves copying type names, and makes freeing builtin types easier.

Ref: ffi#991 Write barrier protected objects are allowed to be promoted to the old generation, which means they only get marked on major GC. The downside is that the RB_BJ_WRITE macro MUST be used to set references, otherwise the referenced object may be garbaged collected. This commit also implement a `dsize` function so that these instance report a more relevant size in various memory profilers. It's not counting everything because some types are opaque right now, so a larger refactoring would be needed.

casperisfine force-pushed the function-write-barrier branch from a5e0a26 to 905fbf3 Compare February 7, 2023 11:27

peterzhu2118 reviewed Feb 7, 2023

View reviewed changes

ext/ffi_c/Function.c Show resolved Hide resolved

casperisfine force-pushed the function-write-barrier branch 3 times, most recently from 7512624 to cb889c6 Compare February 24, 2023 13:47

casperisfine force-pushed the function-write-barrier branch from cb889c6 to 1a2c653 Compare February 24, 2023 14:02

casperisfine pushed a commit to casperisfine/ffi that referenced this pull request Mar 2, 2023

Convert FFI::VariadicInvoker to TypedData

b5bcfca

Ref: ffi#991 The old untyped DATA API is soft deprecated and this new one open the door to write barriers, compaction, memsize etc.

casperisfine mentioned this pull request Mar 2, 2023

Convert FFI::VariadicInvoker to TypedData #994

Merged

casperisfine pushed a commit to casperisfine/ffi that referenced this pull request Mar 2, 2023

Convert FFI::Type and descendants to TypedData

830ea9b

Ref: ffi#991 The old untyped DATA API is soft deprecated and this new one open the door to write barriers, compaction, memsize etc.

casperisfine mentioned this pull request Mar 2, 2023

Convert FFI::Type and descendants to TypedData #995

Merged

casperisfine pushed a commit to casperisfine/ffi that referenced this pull request Mar 2, 2023

Convert FFI::ThreadData to TypedData

902e763

Ref: ffi#991 The old untyped DATA API is soft deprecated and this new one open the door to write barriers, compaction, memsize etc.

casperisfine mentioned this pull request Mar 2, 2023

Convert FFI::ThreadData to TypedData #996

Merged

casperisfine pushed a commit to casperisfine/ffi that referenced this pull request Mar 2, 2023

Convert FFI::Struct::InlineArray to TypedData

f94f3a6

Ref: ffi#991 The old untyped DATA API is soft deprecated and this new one open the door to write barriers, compaction, memsize etc.

casperisfine mentioned this pull request Mar 2, 2023

Convert FFI::Struct::InlineArray to TypedData #997

Merged

casperisfine pushed a commit to casperisfine/ffi that referenced this pull request Mar 2, 2023

Convert FFI::AbstractMemory and descendants to TypedData

ee60e7e

Ref: ffi#991 The old untyped DATA API is soft deprecated and this new one open the door to write barriers, compaction, memsize etc.

casperisfine mentioned this pull request Mar 2, 2023

Convert FFI::AbstractMemory and descendants to TypedData #998

Merged

casperisfine pushed a commit to casperisfine/ffi that referenced this pull request Mar 2, 2023

Convert FFI::AbstractMemory and descendants to TypedData

ca2744a

Ref: ffi#991 The old untyped DATA API is soft deprecated and this new one open the door to write barriers, compaction, memsize etc.

casperisfine pushed a commit to casperisfine/ffi that referenced this pull request Mar 2, 2023

Convert FFI::Struct to TypedData

9e8a4c4

Ref: ffi#991 The old untyped DATA API is soft deprecated and this new one open the door to write barriers, compaction, memsize etc.

casperisfine mentioned this pull request Mar 2, 2023

Convert FFI::Struct to TypedData #999

Merged

casperisfine pushed a commit to casperisfine/ffi that referenced this pull request Mar 2, 2023

Convert FFI::Struct to TypedData

d2f913c

Ref: ffi#991 The old untyped DATA API is soft deprecated and this new one open the door to write barriers, compaction, memsize etc.

larskanis added a commit to larskanis/ffi that referenced this pull request Mar 3, 2023

Convert FFI::DynamicLibrary to TypedData

f7e0004

Ref: ffi#991 The old untyped DATA API is soft deprecated 8000 and this new one open the door to write barriers, compaction, memsize etc.

larskanis mentioned this pull request Mar 3, 2023

Convert FFI::DynamicLibrary to TypedData #1000

Merged

casperisfine mentioned this pull request Mar 6, 2023

Implement Write Barrier and dsize for FFI::FunctionType #1001

Merged

casperisfine closed this Mar 6, 2023

casperisfine mentioned this pull request Mar 6, 2023

Implement Write Barrier and dsize for FFI::Function #1003

Merged

casperisfine mentioned this pull request Mar 6, 2023

Implement Write Barrier and dsize for FFI::DynamicLibrary #1004

Merged

casperisfine mentioned this pull request Mar 6, 2023

Implement Write Barrier and dsize for FFI::Type #1005

Merged

casperisfine mentioned this pull request Mar 6, 2023

Implement the last missing Write Barriers and dsize #1011

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Protect FFI::Function with Write Barrier #991

Protect FFI::Function with Write Barrier #991

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Protect FFI::Function with Write Barrier #991

Protect FFI::Function with Write Barrier #991

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants