From 1c9fb9d45bd7a1a7dbee80dbfa6e06461e4c1e40 Mon Sep 17 00:00:00 2001
From: Joshua Haberman <jhaberman@gmail.com>
Date: Fri, 16 Aug 2019 06:42:13 -0700
Subject: [PATCH 1/5] WIP.

---
 ruby/ext/google/protobuf_c/defs.c          |  3 +++
 ruby/ext/google/protobuf_c/encode_decode.c |  2 +-
 ruby/ext/google/protobuf_c/message.c       |  2 +-
 ruby/ext/google/protobuf_c/protobuf.h      |  3 ++-
 ruby/ext/google/protobuf_c/storage.c       | 23 +++++++++++++++++-----
 5 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/ruby/ext/google/protobuf_c/defs.c b/ruby/ext/google/protobuf_c/defs.c
index 77ef5e554079..63dac7fe1f74 100644
--- a/ruby/ext/google/protobuf_c/defs.c
+++ b/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) {
diff --git a/ruby/ext/google/protobuf_c/encode_decode.c b/ruby/ext/google/protobuf_c/encode_decode.c
index ad8205fea7b9..3bf6b924d947 100644
--- a/ruby/ext/google/protobuf_c/encode_decode.c
+++ b/ruby/ext/google/protobuf_c/encode_decode.c
@@ -690,7 +690,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
diff --git a/ruby/ext/google/protobuf_c/message.c b/ruby/ext/google/protobuf_c/message.c
index dfe24c847e31..5b64374cdba2 100644
--- a/ruby/ext/google/protobuf_c/message.c
+++ b/ruby/ext/google/protobuf_c/message.c
@@ -64,7 +64,7 @@ VALUE Message_alloc(VALUE klass) {
   VALUE ret;
 
   if (desc->layout == NULL) {
-    desc->layout = create_layout(desc);
+    create_layout(desc);
   }
 
   msg = (MessageHeader*)ALLOC_N(uint8_t,
diff --git a/ruby/ext/google/protobuf_c/protobuf.h b/ruby/ext/google/protobuf_c/protobuf.h
index b9d1e5a1026d..84e9e7fd2f25 100644
--- a/ruby/ext/google/protobuf_c/protobuf.h
+++ b/ruby/ext/google/protobuf_c/protobuf.h
@@ -509,11 +509,12 @@ struct MessageField {
 struct MessageLayout {
   const Descriptor* desc;
   const upb_msgdef* msgdef;
+  void* empty_template;  // Can memcpy() onto a layout to clear it.
   MessageField* fields;
   size_t size;
 };
 
-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);
diff --git a/ruby/ext/google/protobuf_c/storage.c b/ruby/ext/google/protobuf_c/storage.c
index 7ce963d15192..375b939598f2 100644
--- a/ruby/ext/google/protobuf_c/storage.c
+++ b/ruby/ext/google/protobuf_c/storage.c
@@ -473,7 +473,7 @@ static size_t align_up_to(size_t offset, size_t granularity) {
   return (offset + granularity - 1) & ~(granularity - 1);
 }
 
-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);
@@ -482,7 +482,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);
 
   for (upb_msg_field_begin(&it, msgdef);
@@ -584,10 +587,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);
 }
@@ -868,15 +880,16 @@ void layout_set(MessageLayout* layout,
   }
 }
 
-void layout_init(MessageLayout* layout,
-                 void* storage) {
-
+void layout_init(MessageLayout* layout, void* storage) {
+  memcpy(storage, layout->empty_template, layout->size);
+  /*
   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));
   }
+  */
 }
 
 void layout_mark(MessageLayout* layout, void* storage) {

From cf07d3c1b20f25debe57439c1511d891492f3d19 Mon Sep 17 00:00:00 2001
From: Joshua Haberman <jhaberman@gmail.com>
Date: Wed, 21 Aug 2019 17:58:37 -0700
Subject: [PATCH 2/5] layout_init() optimization works!

---
 ruby/ext/google/protobuf_c/map.c            |  2 +-
 ruby/ext/google/protobuf_c/message.c        |  2 -
 ruby/ext/google/protobuf_c/protobuf.h       |  6 ++-
 ruby/ext/google/protobuf_c/repeated_field.c |  2 +-
 ruby/ext/google/protobuf_c/storage.c        | 53 +++++++++++++++++++--
 5 files changed, 57 insertions(+), 8 deletions(-)

diff --git a/ruby/ext/google/protobuf_c/map.c b/ruby/ext/google/protobuf_c/map.c
index 4ed80568af1b..5487a576402c 100644
--- a/ruby/ext/google/protobuf_c/map.c
+++ b/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);
diff --git a/ruby/ext/google/protobuf_c/message.c b/ruby/ext/google/protobuf_c/message.c
index 2ba7b2fe476f..4dd68f4a2040 100644
--- a/ruby/ext/google/protobuf_c/message.c
+++ b/ruby/ext/google/protobuf_c/message.c
@@ -70,8 +70,6 @@ VALUE Message_alloc(VALUE klass) {
   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);
diff --git a/ruby/ext/google/protobuf_c/protobuf.h b/ruby/ext/google/protobuf_c/protobuf.h
index 5176d59f0cac..6314b788106c 100644
--- a/ruby/ext/google/protobuf_c/protobuf.h
+++ b/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);
@@ -522,11 +524,13 @@ struct MessageLayout {
   uint32_t size;
   uint32_t value_offset;
   int value_count;
+  int repeated_count;
+  int map_count;
 };
 
 #define ONEOF_CASE_MASK 0x80000000
 
-MessageLayout* create_layout(Descriptor* desc);
+void create_layout(Descriptor* desc);
 void free_layout(MessageLayout* layout);
 bool field_contains_hasbit(MessageLayout* layout,
                  const upb_fielddef* field);
diff --git a/ruby/ext/google/protobuf_c/repeated_field.c b/ruby/ext/google/protobuf_c/repeated_field.c
index 2766087504ff..1c649280fe6d 100644
--- a/ruby/ext/google/protobuf_c/repeated_field.c
+++ b/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);
diff --git a/ruby/ext/google/protobuf_c/storage.c b/ruby/ext/google/protobuf_c/storage.c
index ad76e4f87aa1..6922cdc456f4 100644
--- a/ruby/ext/google/protobuf_c/storage.c
+++ b/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(Descriptor* desc) {
+void create_layout(Descriptor* desc) {
   const upb_msgdef *msgdef = desc->msgdef;
   MessageLayout* layout = ALLOC(MessageLayout);
   int nfields = upb_msgdef_numfields(msgdef);
@@ -517,14 +517,49 @@ MessageLayout* create_layout(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) || !is_value_field(field)) {
+    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) ||
+        upb_fielddef_isseq(field)) {
       continue;
     }
 
@@ -909,7 +944,19 @@ void layout_set(MessageLayout* layout,
 }
 
 void layout_init(MessageLayout* layout, void* storage) {
+  VALUE* value = (VALUE*)CHARPTR_AT(storage, layout->value_offset);
+  int i;
+
   memcpy(storage, layout->empty_template, layout->size);
+
+  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);
+  }
+
   /*
   upb_msg_field_iter it;
   for (upb_msg_field_begin(&it, layout->msgdef);

From b9131f0aabb6df6c9db410bcebb9f1d651d8e31e Mon Sep 17 00:00:00 2001
From: Joshua Haberman <jhaberman@gmail.com>
Date: Wed, 21 Aug 2019 18:05:20 -0700
Subject: [PATCH 3/5] Removed commented-out code.

---
 ruby/ext/google/protobuf_c/storage.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/ruby/ext/google/protobuf_c/storage.c b/ruby/ext/google/protobuf_c/storage.c
index 6922cdc456f4..8705bc9587bf 100644
--- a/ruby/ext/google/protobuf_c/storage.c
+++ b/ruby/ext/google/protobuf_c/storage.c
@@ -956,15 +956,6 @@ void layout_init(MessageLayout* layout, void* storage) {
   for (i = 0; i < layout->map_count; i++, value++) {
     *value = Map_new_this_type(*value);
   }
-
-  /*
-  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));
-  }
-  */
 }
 
 void layout_mark(MessageLayout* layout, void* storage) {

From 3e3407af4946adcaca34990dde6c30305d178fc7 Mon Sep 17 00:00:00 2001
From: Joshua Haberman <jhaberman@gmail.com>
Date: Wed, 21 Aug 2019 18:39:25 -0700
Subject: [PATCH 4/5] Re-add memset() that seemed redundant but is necessary in
 case of GC.

---
 ruby/ext/google/protobuf_c/message.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/ruby/ext/google/protobuf_c/message.c b/ruby/ext/google/protobuf_c/message.c
index 4dd68f4a2040..0e0272fc28ce 100644
--- a/ruby/ext/google/protobuf_c/message.c
+++ b/ruby/ext/google/protobuf_c/message.c
@@ -70,6 +70,9 @@ VALUE Message_alloc(VALUE klass) {
   msg = (MessageHeader*)ALLOC_N(uint8_t,
                                 sizeof(MessageHeader) + desc->layout->size);
 
+  // Required in case a GC happens before layout_init().
+  memset(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);

From 671c2459fc8e8d1ce33f5b81b1991d82b921e8e1 Mon Sep 17 00:00:00 2001
From: Joshua Haberman <jhaberman@gmail.com>
Date: Wed, 28 Aug 2019 13:15:07 -0700
Subject: [PATCH 5/5] Fixed crash bug and moved initialization into init
 method.

---
 ruby/ext/google/protobuf_c/message.c | 21 +++++++++------------
 ruby/ext/google/protobuf_c/storage.c |  2 --
 2 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/ruby/ext/google/protobuf_c/message.c b/ruby/ext/google/protobuf_c/message.c
index 0e0272fc28ce..e3730d280be7 100644
--- a/ruby/ext/google/protobuf_c/message.c
+++ b/ruby/ext/google/protobuf_c/message.c
@@ -62,26 +62,19 @@ VALUE Message_alloc(VALUE klass) {
   Descriptor* desc = ruby_to_Descriptor(descriptor);
   MessageHeader* msg;
   VALUE ret;
+  size_t size;
 
   if (desc->layout == NULL) {
     create_layout(desc);
   }
 
-  msg = (MessageHeader*)ALLOC_N(uint8_t,
-                                sizeof(MessageHeader) + desc->layout->size);
-
-  // Required in case a GC happens before layout_init().
-  memset(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;
 }
@@ -473,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;
diff --git a/ruby/ext/google/protobuf_c/storage.c b/ruby/ext/google/protobuf_c/storage.c
index 8705bc9587bf..861fb0255335 100644
--- a/ruby/ext/google/protobuf_c/storage.c
+++ b/ruby/ext/google/protobuf_c/storage.c
@@ -947,8 +947,6 @@ void layout_init(MessageLayout* layout, void* storage) {
   VALUE* value = (VALUE*)CHARPTR_AT(storage, layout->value_offset);
   int i;
 
-  memcpy(storage, layout->empty_template, layout->size);
-
   for (i = 0; i < layout->repeated_count; i++, value++) {
     *value = RepeatedField_new_this_type(*value);
   }