Merge pull request #6521 from haberman/layout_mark

Optimized layout_mark() for Ruby

ruby/ext/google/protobuf_c/encode_decode.c

@@ -155,6 +155,9 @@ static const void *newoneofhandlerdata(upb_handlers *h,
   // create a separate ID space. In addition, using the field tag number here
   // lets us easily look up the field in the oneof accessor.
   hd->oneof_case_num = upb_fielddef_number(f);
+  if (is_value_field(f)) {
+    hd->oneof_case_num |= ONEOF_CASE_MASK;
+  }
   hd->subklass = field_type_class(desc->layout, f);
   upb_handlers_addcleanup(h, hd, xfree);
   return hd;
@@ -706,12 +709,13 @@ void add_handlers_for_message(const void *closure, upb_handlers *h) {
        !upb_msg_field_done(&i);
        upb_msg_field_next(&i)) {
     const upb_fielddef *f = upb_msg_iter_field(&i);
+    const upb_oneofdef *oneof = upb_fielddef_containingoneof(f);
     size_t offset = desc->layout->fields[upb_fielddef_index(f)].offset +
         sizeof(MessageHeader);
 
-    if (upb_fielddef_containingoneof(f)) {
+    if (oneof) {
       size_t oneof_case_offset =
-          desc->layout->fields[upb_fielddef_index(f)].case_offset +
+          desc->layout->oneofs[upb_oneofdef_index(oneof)].case_offset +
           sizeof(MessageHeader);
       add_handlers_for_oneof_field(h, f, offset, oneof_case_offset, desc);
     } else if (is_map_field(f)) {
@@ -1256,19 +1260,18 @@ static void putmsg(VALUE msg_rb, const Descriptor* desc,
        !upb_msg_field_done(&i);
        upb_msg_field_next(&i)) {
     upb_fielddef *f = upb_msg_iter_field(&i);
+    const upb_oneofdef *oneof = upb_fielddef_containingoneof(f);
     bool is_matching_oneof = false;
     uint32_t offset =
         desc->layout->fields[upb_fielddef_index(f)].offset +
         sizeof(MessageHeader);
 
-    if (upb_fielddef_containingoneof(f)) {
-      uint32_t oneof_case_offset =
-          desc->layout->fields[upb_fielddef_index(f)].case_offset +
-          sizeof(MessageHeader);
+    if (oneof) {
+      uint32_t oneof_case =
+          slot_read_oneof_case(desc->layout, Message_data(msg), oneof);
       // For a oneof, check that this field is actually present -- skip all the
       // below if not.
-      if (DEREF(msg, oneof_case_offset, uint32_t) !=
-          upb_fielddef_number(f)) {
+      if (oneof_case != upb_fielddef_number(f)) {
         continue;
       }
       // Otherwise, fall through to the appropriate singular-field handler
@@ -1464,18 +1467,17 @@ static void discard_unknown(VALUE msg_rb, const Descriptor* desc) {
        !upb_msg_field_done(&it);
        upb_msg_field_next(&it)) {
     upb_fielddef *f = upb_msg_iter_field(&it);
+    const upb_oneofdef *oneof = upb_fielddef_containingoneof(f);
     uint32_t offset =
         desc->layout->fields[upb_fielddef_index(f)].offset +
         sizeof(MessageHeader);
 
-    if (upb_fielddef_containingoneof(f)) {
-      uint32_t oneof_case_offset =
-          desc->layout->fields[upb_fielddef_index(f)].case_offset +
-          sizeof(MessageHeader);
+    if (oneof) {
+      uint32_t oneof_case =
+          slot_read_oneof_case(desc->layout, Message_data(msg), oneof);
       // For a oneof, check that this field is actually present -- skip all the
       // below if not.
-      if (DEREF(msg, oneof_case_offset, uint32_t) !=
-          upb_fielddef_number(f)) {
+      if (oneof_case != upb_fielddef_number(f)) {
         continue;
       }
       // Otherwise, fall through to the appropriate singular-field handler

ruby/ext/google/protobuf_c/message.c

@@ -86,27 +86,11 @@ VALUE Message_alloc(VALUE klass) {
 }
 
 static const upb_fielddef* which_oneof_field(MessageHeader* self, const upb_oneofdef* o) {
-  upb_oneof_iter it;
-  size_t case_ofs;
   uint32_t oneof_case;
-  const upb_fielddef* first_field;
   const upb_fielddef* f;
 
-  // If no fields in the oneof, always nil.
-  if (upb_oneofdef_numfields(o) == 0) {
-    return NULL;
-  }
-  // Grab the first field in the oneof so we can get its layout info to find the
-  // oneof_case field.
-  upb_oneof_begin(&it, o);
-  assert(!upb_oneof_done(&it));
-  first_field = upb_oneof_iter_field(&it);
-  assert(upb_fielddef_containingoneof(first_field) != NULL);
-
-  case_ofs =
-      self->descriptor->layout->
-      fields[upb_fielddef_index(first_field)].case_offset;
-  oneof_case = *((uint32_t*)((char*)Message_data(self) + case_ofs));
+  oneof_case =
+      slot_read_oneof_case(self->descriptor->layout, Message_data(self), o);
 
   if (oneof_case == ONEOF_CASE_NONE) {
     return NULL;

ruby/ext/google/protobuf_c/protobuf.h

@@ -59,6 +59,7 @@ typedef struct OneofDescriptor OneofDescriptor;
 typedef struct EnumDescriptor EnumDescriptor;
 typedef struct MessageLayout MessageLayout;
 typedef struct MessageField MessageField;
+typedef struct MessageOneof MessageOneof;
 typedef struct MessageHeader MessageHeader;
 typedef struct MessageBuilderContext MessageBuilderContext;
 typedef struct OneofBuilderContext OneofBuilderContext;
@@ -367,6 +368,9 @@ bool native_slot_eq(upb_fieldtype_t type, void* mem1, void* mem2);
 
 VALUE native_slot_encode_and_freeze_string(upb_fieldtype_t type, VALUE value);
 void native_slot_check_int_range_precision(const char* name, upb_fieldtype_t type, VALUE value);
+uint32_t slot_read_oneof_case(MessageLayout* layout, const void* storage,
+                              const upb_oneofdef* oneof);
+bool is_value_field(const upb_fielddef* f);
 
 extern rb_encoding* kRubyStringUtf8Encoding;
 extern rb_encoding* kRubyStringASCIIEncoding;
@@ -496,13 +500,16 @@ VALUE Map_iter_value(Map_iter* iter);
 // Message layout / storage.
 // -----------------------------------------------------------------------------
 
-#define MESSAGE_FIELD_NO_CASE ((size_t)-1)
-#define MESSAGE_FIELD_NO_HASBIT ((size_t)-1)
+#define MESSAGE_FIELD_NO_HASBIT ((uint32_t)-1)
 
 struct MessageField {
-  size_t offset;
-  size_t case_offset;  // for oneofs, a uint32. Else, MESSAGE_FIELD_NO_CASE.
-  size_t hasbit;
+  uint32_t offset;
+  uint32_t hasbit;
+};
+
+struct MessageOneof {
+  uint32_t offset;
+  uint32_t case_offset;
 };
 
 // MessageLayout is owned by the enclosing Descriptor, which must outlive us.
@@ -510,9 +517,14 @@ struct MessageLayout {
   const Descriptor* desc;
   const upb_msgdef* msgdef;
   MessageField* fields;
-  size_t size;
+  MessageOneof* oneofs;
+  uint32_t size;
+  uint32_t value_offset;
+  int value_count;
 };
 
+#define ONEOF_CASE_MASK 0x80000000
+
 MessageLayout* create_layout(const Descriptor* desc);
 void free_layout(MessageLayout* layout);
 bool field_contains_hasbit(MessageLayout* layout,