Merge pull request #6547 An error occurred from haberman/layout_clear

Optimization for layout_init()

ruby/ext/google/protobuf_c/defs.c

@@ -487,6 +487,9 @@ void Descriptor_mark(void* _self) {
   Descriptor* self = _self;
   rb_gc_mark(self->klass);
   rb_gc_mark(self->descriptor_pool);
+  if (self->layout && self->layout->empty_template) {
+    layout_mark(self->layout, self->layout->empty_template);
+  }
 }
 
 void Descriptor_free(void* _self) {

ruby/ext/google/protobuf_c/encode_decode.c

@@ -696,7 +696,7 @@ void add_handlers_for_message(const void *closure, upb_handlers *h) {
   // class is actually built, so to work around this, we just create the layout
   // (and handlers, in the class-building function) on-demand.
   if (desc->layout == NULL) {
-    desc->layout = create_layout(desc);
+    create_layout(desc);
   }
 
   // If this is a mapentry message type, set up a special set of handlers and

ruby/ext/google/protobuf_c/map.c

@@ -495,7 +495,7 @@ VALUE Map_length(VALUE _self) {
   return ULL2NUM(upb_strtable_count(&self->table));
 }
 
-static VALUE Map_new_this_type(VALUE _self) {
+VALUE Map_new_this_type(VALUE _self) {
   Map* self = ruby_to_Map(_self);
   VALUE new_map = Qnil;
   VALUE key_type = fieldtype_to_ruby(self->key_type);

ruby/ext/google/protobuf_c/message.c

@@ -62,25 +62,19 @@ VALUE Message_alloc(VALUE klass) {
   Descriptor* desc = ruby_to_Descriptor(descriptor);
   MessageHeader* msg;
   VALUE ret;
+  size_t size;
 
   if (desc->layout == NULL) {
-    desc->layout = create_layout(desc);
+    create_layout(desc);
   }
 
-  msg = (MessageHeader*)ALLOC_N(uint8_t,
-                                sizeof(MessageHeader) + desc->layout->size);
-
-  memset(Message_data(msg), 0, desc->layout->size);
-
-  // We wrap first so that everything in the message object is GC-rooted in case
-  // a collection happens during object creation in layout_init().
-  ret = TypedData_Wrap_Struct(klass, &Message_type, msg);
+  msg = ALLOC_N(uint8_t, sizeof(MessageHeader) + desc->layout->size);
   msg->descriptor = desc;
-  rb_ivar_set(ret, descriptor_instancevar_interned, descriptor);
-
   msg->unknown_fields = NULL;
+  memcpy(Message_data(msg), desc->layout->empty_template, desc->layout->size);
 
-  layout_init(desc->layout, Message_data(msg));
+  ret = TypedData_Wrap_Struct(klass, &Message_type, msg);
+  rb_ivar_set(ret, descriptor_instancevar_interned, descriptor);
 
   return ret;
 }
@@ -472,7 +466,11 @@ int Message_initialize_kwarg(VALUE key, VALUE val, VALUE _self) {
  * Message class are provided on each concrete message class.
  */
 VALUE Message_initialize(int argc, VALUE* argv, VALUE _self) {
+  MessageHeader* self;
   VALUE hash_args;
+  TypedData_Get_Struct(_self, MessageHeader, &Message_type, self);
+
+  layout_init(self->descriptor->layout, Message_data(self));
 
   if (argc == 0) {
     return Qnil;

ruby/ext/google/protobuf_c/protobuf.h

@@ -419,6 +419,7 @@ extern VALUE cRepeatedField;
 
 RepeatedField* ruby_to_RepeatedField(VALUE value);
 
+VALUE RepeatedField_new_this_type(VALUE _self);
 VALUE RepeatedField_each(VALUE _self);
 VALUE RepeatedField_index(int argc, VALUE* argv, VALUE _self);
 void* RepeatedField_index_native(VALUE _self, int index);
@@ -467,6 +468,7 @@ extern VALUE cMap;
 
 Map* ruby_to_Map(VALUE value);
 
+VALUE Map_new_this_type(VALUE _self);
 VALUE Map_each(VALUE _self);
 VALUE Map_keys(VALUE _self);
 VALUE Map_values(VALUE _self);
@@ -516,16 +518,19 @@ struct MessageOneof {
 struct MessageLayout {
   const Descriptor* desc;
   const upb_msgdef* msgdef;
+  void* empty_template;  // Can memcpy() onto a layout to clear it.
   MessageField* fields;
   MessageOneof* oneofs;
   uint32_t size;
   uint32_t value_offset;
   int value_count;
+  int repeated_count;
+  int map_count;
 };
 
 #define ONEOF_CASE_MASK 0x80000000
 
-MessageLayout* create_layout(const Descriptor* desc);
+void create_layout(Descriptor* desc);
 void free_layout(MessageLayout* layout);
 bool field_contains_hasbit(MessageLayout* layout,
                  const upb_fielddef* field);

ruby/ext/google/protobuf_c/repeated_field.c

@@ -323,7 +323,7 @@ VALUE RepeatedField_length(VALUE _self) {
   return INT2NUM(self->size);
 }
 
-static VALUE RepeatedField_new_this_type(VALUE _self) {
+VALUE RepeatedField_new_this_type(VALUE _self) {
   RepeatedField* self = ruby_to_RepeatedField(_self);
   VALUE new_rptfield = Qnil;
   VALUE element_type = fieldtype_to_ruby(self->field_type);

ruby/ext/google/protobuf_c/storage.c

@@ -478,7 +478,7 @@ bool is_value_field(const upb_fielddef* f) {
          upb_fielddef_isstring(f);
 }
 
-MessageLayout* create_layout(const Descriptor* desc) {
+void create_layout(Descriptor* desc) {
   const upb_msgdef *msgdef = desc->msgdef;
   MessageLayout* layout = ALLOC(MessageLayout);
   int nfields = upb_msgdef_numfields(msgdef);
@@ -488,7 +488,10 @@ MessageLayout* create_layout(const Descriptor* desc) {
   size_t off = 0;
   size_t hasbit = 0;
 
+  layout->empty_template = NULL;
   layout->desc = desc;
+  desc->layout = layout;
+
   layout->fields = ALLOC_N(MessageField, nfields);
   layout->oneofs = NULL;
 
@@ -514,14 +517,49 @@ MessageLayout* create_layout(const Descriptor* desc) {
 
   off = align_up_to(off, sizeof(VALUE));
   layout->value_offset = off;
+  layout->repeated_count = 0;
+  layout->map_count = 0;
   layout->value_count = 0;
 
-  // Place all (non-oneof) VALUE fields first.
+  // Place all VALUE fields for repeated fields.
+  for (upb_msg_field_begin(&it, msgdef);
+       !upb_msg_field_done(&it);
+       upb_msg_field_next(&it)) {
+    const upb_fielddef* field = upb_msg_iter_field(&it);
+    if (upb_fielddef_containingoneof(field) || !upb_fielddef_isseq(field) ||
+        upb_fielddef_ismap(field)) {
+      continue;
+    }
+
+    layout->fields[upb_fielddef_index(field)].offset = off;
+    off += sizeof(VALUE);
+    layout->repeated_count++;
+  }
+
+  // Place all VALUE fields for map fields.
+  for (upb_msg_field_begin(&it, msgdef);
+       !upb_msg_field_done(&it);
+       upb_msg_field_next(&it)) {
+    const upb_fielddef* field = upb_msg_iter_field(&it);
+    if (upb_fielddef_containingoneof(field) || !upb_fielddef_isseq(field) ||
+        !upb_fielddef_ismap(field)) {
+      continue;
+    }
+
+    layout->fields[upb_fielddef_index(field)].offset = off;
+    off += sizeof(VALUE);
+    layout->map_count++;
+  }
+
+  layout->value_count = layout->repeated_count + layout->map_count;
+
+  // Next place all other (non-oneof) VALUE fields.
   for (upb_msg_field_begin(&it, msgdef);
        !upb_msg_field_done(&it);
        upb_msg_field_next(&it)) {
     const upb_fielddef* field = upb_msg_iter_field(&it);
-    if (upb_fielddef_containingoneof(field) || !is_value_field(field)) {
+    if (upb_fielddef_containingoneof(field) || !is_value_field(field) ||
+        upb_fielddef_isseq(field)) {
       continue;
     }
 
@@ -600,10 +638,19 @@ MessageLayout* create_layout(const Descriptor* desc) {
   layout->size = off;
   layout->msgdef = msgdef;
 
-  return layout;
+  // Create the empty message template.
+  layout->empty_template = ALLOC_N(char, layout->size);
+  memset(layout->empty_template, 0, layout->size);
+
+  for (upb_msg_field_begin(&it, layout->msgdef);
+       !upb_msg_field_done(&it);
+       upb_msg_field_next(&it)) {
+    layout_clear(layout, layout->empty_template, upb_msg_iter_field(&it));
+  }
 }
 
 void free_layout(MessageLayout* layout) {
+  xfree(layout->empty_template);
   xfree(layout->fields);
   xfree(layout->oneofs);
   xfree(layout);
@@ -896,14 +943,16 @@ void layout_set(MessageLayout* layout,
   }
 }
 
-void layout_init(MessageLayout* layout,
-                 void* storage) {
+void layout_init(MessageLayout* layout, void* storage) {
+  VALUE* value = (VALUE*)CHARPTR_AT(storage, layout->value_offset);
+  int i;
 
-  upb_msg_field_iter it;
-  for (upb_msg_field_begin(&it, layout->msgdef);
-       !upb_msg_field_done(&it);
-       upb_msg_field_next(&it)) {
-    layout_clear(layout, storage, upb_msg_iter_field(&it));
+  for (i = 0; i < layout->repeated_count; i++, value++) {
+    *value = RepeatedField_new_this_type(*value);
+  }
+
+  for (i = 0; i < layout->map_count; i++, value++) {
+    *value = Map_new_this_type(*value);
   }
 }