Change RStruct to match SHAPE_ID_LAYOUT_EXTENDED

THe downside is that we now always reserve one 8B reference for the
IMEMO/fields, even for structs that never have any ivars.

But it is an acceptable tradeoff given that ivars on structs aren't
rare.

Co-Authored-By: John Hawthorn <john@hawthorn.email>

gc.c

@@ -3135,13 +3135,10 @@ static inline bool
 rb_obj_using_gen_fields_table_p(VALUE obj)
 {
     switch (BUILTIN_TYPE(obj)) {
+      case T_STRUCT:
       case T_DATA:
         return false;
 
-      case T_STRUCT:
-        if (!FL_TEST_RAW(obj, RSTRUCT_GEN_FIELDS)) return false;
-        break;
-
       default:
         break;
     }
@@ -3335,9 +3332,7 @@ rb_gc_mark_children(void *objspace, VALUE obj)
             gc_mark_internal(ptr[i]);
         }
 
-        if (rb_obj_shape_has_fields(obj) && !FL_TEST_RAW(obj, RSTRUCT_GEN_FIELDS)) {
-            gc_mark_internal(RSTRUCT_FIELDS_OBJ(obj));
-        }
+        gc_mark_internal(RSTRUCT_FIELDS_OBJ(obj));
 
         break;
       }
@@ -4287,14 +4282,7 @@ rb_gc_update_object_references(void *objspace, VALUE obj)
                 UPDATE_IF_MOVED(objspace, ptr[i]);
             }
 
-            if (RSTRUCT_EMBED_LEN(obj)) {
-                if (!FL_TEST_RAW(obj, RSTRUCT_GEN_FIELDS)) {
-                    UPDATE_IF_MOVED(objspace, ptr[len]);
-                }
-            }
-            else {
-                UPDATE_IF_MOVED(objspace, RSTRUCT(obj)->as.heap.fields_obj);
-            }
+            UPDATE_IF_MOVED(objspace, RSTRUCT(obj)->fields_obj);
         }
         break;
       default:

internal/range.h

@@ -10,6 +10,9 @@
  */
 #include "internal/struct.h"    /* for RSTRUCT */
 
+#define RANGE_FL_INIT FL_USER18
+#define RANGE_FL_EXCL FL_USER19
+
 /* range.c */
 static inline VALUE RANGE_BEG(VALUE r);
 static inline VALUE RANGE_END(VALUE r);
@@ -30,7 +33,10 @@ RANGE_END(VALUE r)
 static inline VALUE
 RANGE_EXCL(VALUE r)
 {
-    return RSTRUCT_GET_RAW(r, 2);
+    if (FL_TEST_RAW(r, RANGE_FL_INIT)) {
+        return RBOOL(FL_TEST_RAW(r, RANGE_FL_EXCL));
+    }
+    return Qnil;
 }
 
 VALUE

internal/struct.h

@@ -17,26 +17,20 @@
  *          If non-zero, the struct is embedded (its contents follow the
  *          header, rather than being on a separately allocated buffer) and
  *          these bits are the length of the Struct.
- * 8:   RSTRUCT_GEN_FIELDS
- *          The struct is embedded and has no space left to store the
- *          IMEMO/fields reference. Any ivar this struct may have will be in
- *          the generic_fields_tbl. This flag doesn't imply the struct has
- *          ivars.
  */
 enum {
     RSTRUCT_EMBED_LEN_MASK = RUBY_FL_USER7 | RUBY_FL_USER6 | RUBY_FL_USER5 | RUBY_FL_USER4 |
                                  RUBY_FL_USER3 | RUBY_FL_USER2 | RUBY_FL_USER1,
     RSTRUCT_EMBED_LEN_SHIFT = (RUBY_FL_USHIFT+1),
-    RSTRUCT_GEN_FIELDS = RUBY_FL_USER8,
 };
 
 struct RStruct {
     struct RBasic basic;
+    VALUE fields_obj;
     union {
         struct {
             long len;
             const VALUE *ptr;
-            VALUE fields_obj;
         } heap;
         /* This is a length 1 array because:
          *   1. GCC has a bug that does not optimize C flexible array members
@@ -113,28 +107,12 @@ RSTRUCT_GET_RAW(VALUE st, long k)
 static inline VALUE
 RSTRUCT_FIELDS_OBJ(VALUE st)
 {
-    const long embed_len = RSTRUCT_EMBED_LEN(st);
-    VALUE fields_obj;
-    if (embed_len) {
-        RUBY_ASSERT(!FL_TEST_RAW(st, RSTRUCT_GEN_FIELDS));
-        fields_obj = RSTRUCT_GET_RAW(st, embed_len);
-    }
-    else {
-        fields_obj = RSTRUCT(st)->as.heap.fields_obj;
-    }
-    return fields_obj;
+    return RSTRUCT(st)->fields_obj;
 }
 
 static inline void
 RSTRUCT_SET_FIELDS_OBJ(VALUE st, VALUE fields_obj)
 {
-    const long embed_len = RSTRUCT_EMBED_LEN(st);
-    if (embed_len) {
-        RUBY_ASSERT(!FL_TEST_RAW(st, RSTRUCT_GEN_FIELDS));
-        RSTRUCT_SET_RAW(st, embed_len, fields_obj);
-    }
-    else {
-        RB_OBJ_WRITE(st, &RSTRUCT(st)->as.heap.fields_obj, fields_obj);
-    }
+    RB_OBJ_WRITE(st, &RSTRUCT(st)->fields_obj, fields_obj);
 }
 #endif /* INTERNAL_STRUCT_H */

range.c

@@ -40,9 +40,8 @@ static VALUE r_cover_p(VALUE, VALUE, VALUE, VALUE);
 
 #define RANGE_SET_BEG(r, v) (RSTRUCT_SET(r, 0, v))
 #define RANGE_SET_END(r, v) (RSTRUCT_SET(r, 1, v))
-#define RANGE_SET_EXCL(r, v) (RSTRUCT_SET(r, 2, v))
 
-#define EXCL(r) RTEST(RANGE_EXCL(r))
+#define EXCL(r) RTEST(FL_TEST(r, RANGE_FL_EXCL))
 
 static void
 range_init(VALUE range, VALUE beg, VALUE end, VALUE exclude_end)
@@ -56,7 +55,12 @@ range_init(VALUE range, VALUE beg, VALUE end, VALUE exclude_end)
             rb_raise(rb_eArgError, "bad value for range");
     }
 
-    RANGE_SET_EXCL(range, exclude_end);
+    if (RTEST(exclude_end)) {
+        FL_SET_RAW(range, RANGE_FL_EXCL);
+    }
+
+    FL_SET_RAW(range, RANGE_FL_INIT);
+
     RANGE_SET_BEG(range, beg);
     RANGE_SET_END(range, end);
 
@@ -79,7 +83,7 @@ range_modify(VALUE range)
 {
     rb_check_frozen(range);
     /* Ranges are immutable, so that they should be initialized only once. */
-    if (RANGE_EXCL(range) != Qnil) {
+    if (FL_TEST(range, RANGE_FL_INIT)) {
         rb_name_err_raise("'initialize' called twice", range, ID2SYM(idInitialize));
     }
 }
@@ -115,6 +119,7 @@ static VALUE
 range_initialize_copy(VALUE range, VALUE orig)
 {
     range_modify(range);
+    FL_SET_RAW(range, FL_TEST_RAW(orig, RANGE_FL_EXCL|RANGE_FL_INIT));
     rb_struct_init_copy(range, orig);
     return range;
 }
@@ -2960,7 +2965,7 @@ Init_Range(void)
 
     rb_cRange = rb_struct_define_without_accessor(
         "Range", rb_cObject, range_alloc,
-        "begin", "end", "excl", NULL);
+        "begin", "end", NULL);
 
     rb_include_module(rb_cRange, rb_mEnumerable);
     rb_marshal_define_compat(rb_cRange, rb_cObject, range_dumper, range_loader);

shape.c

@@ -1219,19 +1219,24 @@ rb_shape_expected_layout(VALUE obj)
       case T_OBJECT: {
           return SHAPE_ID_LAYOUT_ROBJECT;
       }
+
       case T_CLASS:
       case T_MODULE:
         if (FL_TEST_RAW(obj, RCLASS_BOXABLE)) {
             return SHAPE_ID_LAYOUT_OTHER;
         }
         return SHAPE_ID_LAYOUT_RCLASS;
+
+      case T_STRUCT:
       case T_DATA:
         return SHAPE_ID_LAYOUT_EXTENDED;
+
       case T_IMEMO:
         if (IMEMO_TYPE_P(obj, imemo_fields)) {
             return SHAPE_ID_LAYOUT_ROBJECT;
         }
         return SHAPE_ID_LAYOUT_OTHER;
+
       default:
         return SHAPE_ID_LAYOUT_OTHER;
     }

shape.h

@@ -107,7 +107,7 @@ typedef uint32_t redblack_id_t;
 enum shape_type {
     SHAPE_ROOT,
     SHAPE_IVAR,
-    SHAPE_OBJ_ID,
+    SHAPE_OBJ_ID
 };
 
 struct rb_shape {

struct.c

@@ -818,6 +818,8 @@ struct_heap_alloc(VALUE st, size_t len)
     return ALLOC_N(VALUE, len);
 }
 
+STATIC_ASSERT(robject_rstruct_fields_offset, offsetof(struct RObject, as.extended) == offsetof(struct RStruct, fields_obj));
+
 static VALUE
 struct_alloc(VALUE klass)
 {
@@ -833,40 +835,26 @@ struct_alloc(VALUE klass)
 
     if (n > 0 && n <= embed_len_max && rb_gc_size_allocatable_p(embedded_size)) {
         flags |= n << RSTRUCT_EMBED_LEN_SHIFT;
-        if (RCLASS_MAX_IV_COUNT(klass) == 0) {
-            // We set the flag before calling `NEWOBJ_OF` in case a NEWOBJ tracepoint does
-            // attempt to write fields. We'll remove it later if no fields was written to.
-            flags |= RSTRUCT_GEN_FIELDS;
-        }
 
-        NEWOBJ_OF(st, struct RStruct, klass, flags, embedded_size);
-        if (RCLASS_MAX_IV_COUNT(klass) == 0) {
-            if (!rb_obj_shape_has_fields((VALUE)st)
-                    && embedded_size < rb_obj_shape_slot_size((VALUE)st)) {
-                FL_UNSET_RAW((VALUE)st, RSTRUCT_GEN_FIELDS);
-                RSTRUCT_SET_FIELDS_OBJ((VALUE)st, 0);
-            }
-        }
-        else {
-            RSTRUCT_SET_FIELDS_OBJ((VALUE)st, 0);
-        }
+        VALUE st = rb_newobj(GET_EC(), klass, flags, ROOT_SHAPE_ID | SHAPE_ID_LAYOUT_EXTENDED, true, embedded_size);
+        RSTRUCT_SET_FIELDS_OBJ(st, 0);
+        rb_mem_clear((VALUE *)RSTRUCT(st)->as.ary, n);
 
-        rb_mem_clear((VALUE *)st->as.ary, n);
-
-        return (VALUE)st;
+        return st;
     }
     else {
-        NEWOBJ_OF(st, struct RStruct, klass, flags, sizeof(struct RStruct));
+        VALUE obj = rb_newobj(GET_EC(), klass, flags, ROOT_SHAPE_ID | SHAPE_ID_LAYOUT_EXTENDED, true, sizeof(struct RStruct));
+        struct RStruct *st = RSTRUCT(obj);
 
+        st->fields_obj = 0;
         st->as.heap.ptr = NULL;
-        st->as.heap.fields_obj = 0;
         st->as.heap.len = 0;
 
         st->as.heap.ptr = struct_heap_alloc((VALUE)st, n);
         rb_mem_clear((VALUE *)st->as.heap.ptr, n);
         st->as.heap.len = n;
 
-        return (VALUE)st;
+        return obj;
     }
 }
 

test/ruby/test_shapes.rb

@@ -1108,12 +1108,12 @@ class TestShapes < Test::Unit::TestCase
 
     assert_equal :extended_or_rdata, RubyVM::Shape.of(Thread.current).layout
     assert_equal :extended_or_rdata, RubyVM::Shape.of(lambda {}).layout
+    assert_equal :extended_or_rdata, RubyVM::Shape.of(Struct.new(:x).new(1)).layout
+    assert_equal :extended_or_rdata, RubyVM::Shape.of(2..3).layout
 
-    assert_equal :other, RubyVM::Shape.of(Struct.new(:x).new(1)).layout
     assert_equal :other, RubyVM::Shape.of([]).layout
     assert_equal :other, RubyVM::Shape.of("hello").layout
     assert_equal :other, RubyVM::Shape.of(/foo/).layout
-    assert_equal :other, RubyVM::Shape.of(2..3).layout
     assert_equal :other, RubyVM::Shape.of(2**67).layout
     assert_equal :other, RubyVM::Shape.of(:"aaroniscool#{123}").layout
   end

variable.c

@@ -1281,10 +1281,9 @@ obj_use_generic_fields_tbl_p(VALUE obj)
       case T_OBJECT:
       case T_CLASS:
       case T_MODULE:
+      case T_STRUCT:
       case T_DATA:
         return false;
-      case T_STRUCT:
-        return !FL_TEST_RAW(obj, RSTRUCT_GEN_FIELDS);
       default:
         return true;
     }
@@ -1311,12 +1310,9 @@ rb_obj_fields(VALUE obj, ID field_name)
         return RTYPEDDATA(obj)->fields_obj;
 
       case T_STRUCT:
-        if (LIKELY(!FL_TEST_RAW(obj, RSTRUCT_GEN_FIELDS))) {
-            return RSTRUCT_FIELDS_OBJ(obj);
-        }
-        goto generic_fields;
+        return RSTRUCT_FIELDS_OBJ(obj);
+
       default:
-      generic_fields:
         {
             VALUE fields_obj = 0;
 
@@ -1348,13 +1344,10 @@ rb_free_generic_ivar(VALUE obj)
             RB_OBJ_WRITE(obj, &RTYPEDDATA(obj)->fields_obj, 0);
             break;
           case T_STRUCT:
-            if (LIKELY(!FL_TEST_RAW(obj, RSTRUCT_GEN_FIELDS))) {
-                RSTRUCT_SET_FIELDS_OBJ(obj, 0);
-                break;
-            }
-            goto generic_fields;
+            RSTRUCT_SET_FIELDS_OBJ(obj, 0);
+            break;
+
           default:
-          generic_fields:
             {
                 // Other EC may have stale caches, so fields_obj should be
                 // invalidated and the GC will replace with Qundef
@@ -1400,13 +1393,10 @@ rb_obj_set_fields(VALUE obj, VALUE fields_obj, ID field_name, VALUE original_fie
             RB_OBJ_WRITE(obj, &RTYPEDDATA(obj)->fields_obj, fields_obj);
             break;
           case T_STRUCT:
-            if (LIKELY(!FL_TEST_RAW(obj, RSTRUCT_GEN_FIELDS))) {
-                RSTRUCT_SET_FIELDS_OBJ(obj, fields_obj);
-                break;
-            }
-            goto generic_fields;
+            RSTRUCT_SET_FIELDS_OBJ(obj, fields_obj);
+            break;
+
           default:
-          generic_fields:
             {
                 RB_VM_LOCKING() {
                     st_insert(generic_fields_tbl_, (st_data_t)obj, (st_data_t)fields_obj);

yjit/src/cruby.rs

@@ -761,8 +761,8 @@ mod manual_defs {
     pub const RUBY_OFFSET_RARRAY_AS_HEAP_PTR: i32 = 32; // struct RArray, subfield "as.heap.ptr"
     pub const RUBY_OFFSET_RARRAY_AS_ARY: i32 = 16; // struct RArray, subfield "as.ary"
 
-    pub const RUBY_OFFSET_RSTRUCT_AS_HEAP_PTR: i32 = 24; // struct RStruct, subfield "as.heap.ptr"
-    pub const RUBY_OFFSET_RSTRUCT_AS_ARY: i32 = 16; // struct RStruct, subfield "as.ary"
+    pub const RUBY_OFFSET_RSTRUCT_AS_HEAP_PTR: i32 = 32; // struct RStruct, subfield "as.heap.ptr"
+    pub const RUBY_OFFSET_RSTRUCT_AS_ARY: i32 = 24; // struct RStruct, subfield "as.ary"
 
     pub const RUBY_OFFSET_RSTRING_AS_HEAP_PTR: i32 = 24; // struct RString, subfield "as.heap.ptr"
     pub const RUBY_OFFSET_RSTRING_AS_ARY: i32 = 24; // struct RString, subfield "as.embed.ary"

zjit/src/cruby.rs

@@ -1212,8 +1212,8 @@ mod manual_defs {
     pub const RUBY_OFFSET_RARRAY_AS_HEAP_PTR: i32 = 32; // struct RArray, subfield "as.heap.ptr"
     pub const RUBY_OFFSET_RARRAY_AS_ARY: i32 = 16; // struct RArray, subfield "as.ary"
 
-    pub const RUBY_OFFSET_RSTRUCT_AS_HEAP_PTR: i32 = 24; // struct RStruct, subfield "as.heap.ptr"
-    pub const RUBY_OFFSET_RSTRUCT_AS_ARY: i32 = 16; // struct RStruct, subfield "as.ary"
+    pub const RUBY_OFFSET_RSTRUCT_AS_HEAP_PTR: i32 = 32; // struct RStruct, subfield "as.heap.ptr"
+    pub const RUBY_OFFSET_RSTRUCT_AS_ARY: i32 = 24; // struct RStruct, subfield "as.ary"
 
     pub const RUBY_OFFSET_RSTRING_AS_HEAP_PTR: i32 = 24; // struct RString, subfield "as.heap.ptr"
     pub const RUBY_OFFSET_RSTRING_AS_ARY: i32 = 24; // struct RString, subfield "as.embed.ary"

zjit/src/hir/opt_tests.rs

@@ -9352,8 +9352,8 @@ mod hir_opt_tests {
           v11:HeapBasicObject = GuardType v6, HeapBasicObject
           v12:CShape = LoadField v11, :shape_id@0x1000
           v13:CShape[0x1001] = GuardBitEquals v12, CShape(0x1001) recompile
-          v14:CAttrIndex[0] = Const CAttrIndex(0)
-          v15:BasicObject = CCall v11, :rb_ivar_get_at_no_ractor_check@0x1002, v14
+          v14:RubyValue = LoadField v11, :fields_obj@0x1002
+          v15:BasicObject = LoadField v14, :@a@0x1002
           CheckInterrupts
           Return v15
         ");