Merge branch 'struct-layout-write-barrier' of https://github.com/casperisfine/ffi into casperisfine-struct-layout-write-barrier

5 files changed, 70 insertions, 16 deletions

diff --git a/ext/ffi_c/Struct.c b/ext/ffi_c/Struct.c
index 2069096..4f57cf3 100644
--- a/ext/ffi_c/Struct.c
+++ b/ext/ffi_c/Struct.c
@@ -317,7 +317,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 c28f372..0ce5273 100644
--- a/spec/ffi/struct_spec.rb
+++ b/spec/ffi/struct_spec.rb
@@ -1027,7 +1027,7 @@ describe "variable-length arrays" do
   end
 end
 
-describe "Struct memsize", skip: RUBY_ENGINE != "ruby" do
+describe "Struct memsize functions", skip: RUBY_ENGINE != "ruby" do
   it "has a memsize function" do
     base_size = ObjectSpace.memsize_of(Object.new)
 
@@ -1038,8 +1038,40 @@ describe "Struct memsize", skip: RUBY_ENGINE != "ruby" do
     size = ObjectSpace.memsize_of(struct)
     expect(size).to be > base_size
   end
+
+  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