From cae3095fd1c782660baf5aa1f8aa40415d032799 Mon Sep 17 00:00:00 2001 From: Jean Boussier Date: Mon, 6 Mar 2023 15:03:56 +0100 Subject: Implement Write Barrier and dsize for FFI::StructLayout And FFI::StructLayout::Field 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. I had to get rid of the `memset(0)` in `struct_layout_mark` has with Write Barriers it's not guaranteed that the layout will be marked before `fieldName` is moved. I don't think it was a good fix anyway, it's better to mark these VALUE so the GC pin them. I think we could go back to an `st_table` and mark only the keys with `rb_mark_set`. But I didn't want to go down this rabbit hole. --- ext/ffi_c/Struct.c | 2 +- ext/ffi_c/Struct.h | 3 ++- ext/ffi_c/StructLayout.c | 44 ++++++++++++++++++++++++++++++++------------ lib/ffi/struct.rb | 3 ++- spec/ffi/struct_spec.rb | 34 ++++++++++++++++++++++++++++++++++ 5 files changed, 71 insertions(+), 15 deletions(-) diff --git a/ext/ffi_c/Struct.c b/ext/ffi_c/Struct.c index f2466c8..6144018 100644 --- a/ext/ffi_c/Struct.c +++ b/ext/ffi_c/Struct.c @@ -304,7 +304,7 @@ struct_field(Struct* s, VALUE fieldName) rb_raise(rb_eArgError, "No such field '%s'", StringValueCStr(str)); } /* Write the retrieved coder to the cache */ - p_ce->fieldName = fieldName; + RB_OBJ_WRITE(s->rbLayout, &p_ce->fieldName, fieldName); TypedData_Get_Struct(rbField, StructField, &rbffi_struct_field_data_type, p_ce->field); } diff --git a/ext/ffi_c/Struct.h b/ext/ffi_c/Struct.h index 1cba8bb..8e9491b 100644 --- a/ext/ffi_c/Struct.h +++ b/ext/ffi_c/Struct.h @@ -78,11 +78,12 @@ extern "C" { * This avoids full ruby hash lookups for repeated lookups. */ #define FIELD_CACHE_LOOKUP(this, sym) ( &(this)->cache_row[((sym) >> 8) & 0xff] ) + #define FIELD_CACHE_ROWS 0x100 struct field_cache_entry { VALUE fieldName; StructField *field; - } cache_row[0x100]; + } cache_row[FIELD_CACHE_ROWS]; /** The number of reference tracking fields in this struct */ int referenceFieldCount; diff --git a/ext/ffi_c/StructLayout.c b/ext/ffi_c/StructLayout.c index 2371de6..f04b11f 100644 --- a/ext/ffi_c/StructLayout.c +++ b/ext/ffi_c/StructLayout.c @@ -53,7 +53,9 @@ static void struct_layout_mark(void *); static void struct_layout_free(void *); +static size_t struct_layout_memsize(const void *); static void struct_field_mark(void *); +static size_t struct_field_memsize(const void *); VALUE rbffi_StructLayoutFieldClass = Qnil; VALUE rbffi_StructLayoutNumberFieldClass = Qnil, rbffi_StructLayoutPointerFieldClass = Qnil; @@ -67,10 +69,12 @@ const rb_data_type_t rbffi_struct_layout_data_type = { /* extern */ .function = { .dmark = struct_layout_mark, .dfree = struct_layout_free, - .dsize = NULL, + .dsize = struct_layout_memsize, }, .parent = &rbffi_type_data_type, - .flags = RUBY_TYPED_FREE_IMMEDIATELY + // IMPORTANT: WB_PROTECTED objects must only use the RB_OBJ_WRITE() + // macro to update VALUE references, as to trigger write barriers. + .flags = RUBY_TYPED_FREE_IMMEDIATELY | RUBY_TYPED_WB_PROTECTED }; const rb_data_type_t rbffi_struct_field_data_type = { /* extern */ @@ -78,10 +82,12 @@ const rb_data_type_t rbffi_struct_field_data_type = { /* extern */ .function = { .dmark = struct_field_mark, .dfree = RUBY_TYPED_DEFAULT_FREE, - .dsize = NULL, + .dsize = struct_field_memsize, }, .parent = &rbffi_type_data_type, - .flags = RUBY_TYPED_FREE_IMMEDIATELY + // IMPORTANT: WB_PROTECTED objects must only use the RB_OBJ_WRITE() + // macro to update VALUE references, as to trigger write barriers. + .flags = RUBY_TYPED_FREE_IMMEDIATELY | RUBY_TYPED_WB_PROTECTED }; static VALUE @@ -91,8 +97,8 @@ struct_field_allocate(VALUE klass) VALUE obj; obj = TypedData_Make_Struct(klass, StructField, &rbffi_struct_field_data_type, field); - field->rbType = Qnil; - field->rbName = Qnil; + RB_OBJ_WRITE(obj, &field->rbType, Qnil); + RB_OBJ_WRITE(obj, &field->rbName, Qnil); return obj; } @@ -105,6 +111,12 @@ struct_field_mark(void *data) rb_gc_mark(f->rbName); } +static size_t +struct_field_memsize(const void *data) +{ + return sizeof(StructField); +} + /* * call-seq: initialize(name, offset, type) * @param [String,Symbol] name @@ -137,8 +149,8 @@ struct_field_initialize(int argc, VALUE* argv, VALUE self) } field->offset = NUM2UINT(rbOffset); - field->rbName = (TYPE(rbName) == T_SYMBOL) ? rbName : rb_str_intern(rbName); - field->rbType = rbType; + RB_OBJ_WRITE(self, &field->rbName, (TYPE(rbName) == T_SYMBOL) ? rbName : rb_str_intern(rbName)); + RB_OBJ_WRITE(self, &field->rbType, rbType); TypedData_Get_Struct(field->rbType, Type, &rbffi_type_data_type, field->type); field->memoryOp = get_memory_op(field->type); field->referenceIndex = -1; @@ -619,10 +631,9 @@ struct_layout_mark(void *data) rb_gc_mark(layout->rbFieldMap); rb_gc_mark(layout->rbFieldNames); rb_gc_mark(layout->rbFields); - /* Clear the cache, to be safe from changes of fieldName VALUE by GC.compact. - * TODO: Move cache clearing to compactation callback provided by Ruby-2.7+. - */ - memset(&layout->cache_row, 0, sizeof(layout->cache_row)); + for (size_t index = 0; index < FIELD_CACHE_ROWS; index++) { + rb_gc_mark(layout->cache_row[index].fieldName); + } } static void @@ -635,6 +646,15 @@ struct_layout_free(void *data) xfree(layout); } +static size_t +struct_layout_memsize(const void * data) +{ + const StructLayout *layout = (const StructLayout *)data; + size_t memsize = sizeof(StructLayout); + memsize += layout->fieldCount * (sizeof(StructField *) + sizeof(ffi_type *)); + memsize += sizeof(*layout->base.ffiType); + return memsize; +} void rbffi_StructLayout_Init(VALUE moduleFFI) diff --git a/lib/ffi/struct.rb b/lib/ffi/struct.rb index 7028258..725b9cb 100644 --- a/lib/ffi/struct.rb +++ b/lib/ffi/struct.rb @@ -203,9 +203,10 @@ module FFI # :field3, :string # end def layout(*spec) - warn "[DEPRECATION] Struct layout is already defined for class #{self.inspect}. Redefinition as in #{caller[0]} will be disallowed in ffi-2.0." if defined?(@layout) return @layout if spec.size == 0 + warn "[DEPRECATION] Struct layout is already defined for class #{self.inspect}. Redefinition as in #{caller[0]} will be disallowed in ffi-2.0." if defined?(@layout) + builder = StructLayoutBuilder.new builder.union = self < Union builder.packed = @packed if defined?(@packed) diff --git a/spec/ffi/struct_spec.rb b/spec/ffi/struct_spec.rb index fb574d2..c348db7 100644 --- a/spec/ffi/struct_spec.rb +++ b/spec/ffi/struct_spec.rb @@ -1027,6 +1027,40 @@ describe "variable-length arrays" do end end +describe "Struct memsize functions", skip: RUBY_ENGINE != "ruby" do + class SmallCustomStruct < FFI::Struct + layout :pointer, :pointer + end + + class LargerCustomStruct < FFI::Struct + layout :pointer, :pointer, + :c, :char, + :i, :int + end + + it "StructLayout has a memsize function" do + base_size = ObjectSpace.memsize_of(Object.new) + + layout = SmallCustomStruct.layout + size = ObjectSpace.memsize_of(layout) + expect(size).to be > base_size + base_size = size + + layout = LargerCustomStruct.layout + size = ObjectSpace.memsize_of(layout) + expect(size).to be > base_size + end + + it "StructField has a memsize function" do + base_size = ObjectSpace.memsize_of(Object.new) + + layout = SmallCustomStruct.layout + size = ObjectSpace.memsize_of(layout[:pointer]) + expect(size).to be > base_size + end +end + + describe "Struct order" do before :all do @struct = Class.new(FFI::Struct) do -- cgit v1.2.1