From 3c1bd7982b93803ab7d37c3bd14730926f228d46 Mon Sep 17 00:00:00 2001 From: Jean Boussier Date: Mon, 6 Mar 2023 13:50:33 +0100 Subject: Implement Write Barrier and dsize for FFI::Type Ref: https://github.com/ffi/ffi/pull/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. --- ext/ffi_c/MappedType.c | 21 +++++++++++++++------ ext/ffi_c/MappedType.h | 1 - ext/ffi_c/Type.c | 39 ++++++++++++++++++++++++--------------- ext/ffi_c/Type.h | 2 +- spec/ffi/type_spec.rb | 43 +++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 83 insertions(+), 23 deletions(-) create mode 100644 spec/ffi/type_spec.rb diff --git a/ext/ffi_c/MappedType.c b/ext/ffi_c/MappedType.c index 304de86..ed05c9d 100644 --- a/ext/ffi_c/MappedType.c +++ b/ext/ffi_c/MappedType.c @@ -39,6 +39,7 @@ static VALUE mapped_allocate(VALUE); static VALUE mapped_initialize(VALUE, VALUE); static void mapped_mark(void *); +static size_t mapped_memsize(const void *); static ID id_native_type, id_to_native, id_from_native; VALUE rbffi_MappedTypeClass = Qnil; @@ -48,10 +49,12 @@ static const rb_data_type_t mapped_type_data_type = { .function = { .dmark = mapped_mark, .dfree = RUBY_TYPED_DEFAULT_FREE, - .dsize = NULL, + .dsize = mapped_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 }; @@ -62,8 +65,8 @@ mapped_allocate(VALUE klass) VALUE obj = TypedData_Make_Struct(klass, MappedType, &mapped_type_data_type, m); - m->rbConverter = Qnil; - m->rbType = Qnil; + RB_OBJ_WRITE(obj, &m->rbConverter, Qnil); + RB_OBJ_WRITE(obj, &m->rbType, Qnil); m->type = NULL; m->base.nativeType = NATIVE_MAPPED; m->base.ffiType = &ffi_type_void; @@ -95,12 +98,12 @@ mapped_initialize(VALUE self, VALUE rbConverter) } TypedData_Get_Struct(self, MappedType, &mapped_type_data_type, m); - m->rbType = rb_funcall2(rbConverter, id_native_type, 0, NULL); + RB_OBJ_WRITE(self, &m->rbType, rb_funcall2(rbConverter, id_native_type, 0, NULL)); if (!(rb_obj_is_kind_of(m->rbType, rbffi_TypeClass))) { rb_raise(rb_eTypeError, "native_type did not return instance of FFI::Type"); } - m->rbConverter = rbConverter; + RB_OBJ_WRITE(self, &m->rbConverter, rbConverter); TypedData_Get_Struct(m->rbType, Type, &rbffi_type_data_type, m->type); m->base.ffiType = m->type->ffiType; @@ -115,6 +118,12 @@ mapped_mark(void* data) rb_gc_mark(m->rbConverter); } +static size_t +mapped_memsize(const void *data) +{ + return sizeof(MappedType); +} + /* * call-seq: mapped_type.native_type * @return [Type] diff --git a/ext/ffi_c/MappedType.h b/ext/ffi_c/MappedType.h index ac86a3c..9f6f9ee 100644 --- a/ext/ffi_c/MappedType.h +++ b/ext/ffi_c/MappedType.h @@ -43,7 +43,6 @@ typedef struct MappedType_ { Type* type; VALUE rbConverter; VALUE rbType; - } MappedType; void rbffi_MappedType_Init(VALUE moduleFFI); diff --git a/ext/ffi_c/Type.c b/ext/ffi_c/Type.c index d940e94..6089a14 100644 --- a/ext/ffi_c/Type.c +++ b/ext/ffi_c/Type.c @@ -39,13 +39,14 @@ #include "Types.h" #include "Type.h" +static size_t type_memsize(const void *); typedef struct BuiltinType_ { Type type; - char* name; + const char* name; } BuiltinType; -static void builtin_type_free(void *); +static size_t builtin_type_memsize(const void *); VALUE rbffi_TypeClass = Qnil; @@ -59,22 +60,32 @@ const rb_data_type_t rbffi_type_data_type = { /* extern */ .function = { .dmark = NULL, .dfree = RUBY_TYPED_DEFAULT_FREE, - .dsize = NULL, + .dsize = type_memsize, }, - .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 const rb_data_type_t builtin_type_data_type = { .wrap_struct_name = "FFI::Type::Builtin", .function = { .dmark = NULL, - .dfree = builtin_type_free, - .dsize = NULL, + .dfree = RUBY_TYPED_DEFAULT_FREE, + .dsize = builtin_type_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 size_t +type_memsize(const void *data) +{ + return sizeof(Type); +} + static VALUE type_allocate(VALUE klass) { @@ -83,7 +94,7 @@ type_allocate(VALUE klass) type->nativeType = -1; type->ffiType = &ffi_type_void; - + return obj; } @@ -170,20 +181,18 @@ builtin_type_new(VALUE klass, int nativeType, ffi_type* ffiType, const char* nam VALUE obj = Qnil; obj = TypedData_Make_Struct(klass, BuiltinType, &builtin_type_data_type, type); - - type->name = strdup(name); + + type->name = name; type->type.nativeType = nativeType; type->type.ffiType = ffiType; return obj; } -static void -builtin_type_free(void *data) +static size_t +builtin_type_memsize(const void *data) { - BuiltinType *type = (BuiltinType *)data; - free(type->name); - xfree(type); + return sizeof(BuiltinType) + sizeof(ffi_type); } /* diff --git a/ext/ffi_c/Type.h b/ext/ffi_c/Type.h index 74a931b..b6de634 100644 --- a/ext/ffi_c/Type.h +++ b/ext/ffi_c/Type.h @@ -44,7 +44,7 @@ extern "C" { typedef struct Type_ Type; #include "Types.h" - + struct Type_ { NativeType nativeType; ffi_type* ffiType; diff --git a/spec/ffi/type_spec.rb b/spec/ffi/type_spec.rb new file mode 100644 index 0000000..eb48a43 --- /dev/null +++ b/spec/ffi/type_spec.rb @@ -0,0 +1,43 @@ +# +# This file is part of ruby-ffi. +# For licensing, see LICENSE.SPECS +# + +require File.expand_path(File.join(File.dirname(__FILE__), "spec_helper")) + +describe "FFI::Type" do + it 'has a memsize function', skip: RUBY_ENGINE != "ruby" do + base_size = ObjectSpace.memsize_of(Object.new) + + size = ObjectSpace.memsize_of(FFI::Type.new(42)) + expect(size).to be > base_size + base_size = size + + converter = Module.new do + extend FFI::DataConverter + + def self.native_type + @native_type_called = true + FFI::Type::INT32 + end + + def self.to_native(val, ctx) + @to_native_called = true + ToNativeMap[val] + end + + def self.from_native(val, ctx) + @from_native_called = true + FromNativeMap[val] + end + end + + size = ObjectSpace.memsize_of(FFI::Type::Mapped.new(converter)) + expect(size).to be > base_size + base_size = size + + # Builtin types are larger as they also have a name and own ffi_type + size = ObjectSpace.memsize_of(FFI::Type::Builtin::CHAR) + expect(size).to be > base_size + end +end -- cgit v1.2.1