Merge pull request #405 from cfallin/ruby-speed

Ruby C extension speedup: don't re-intern constant string needlessly.

ruby/ext/google/protobuf_c/defs.c

@@ -34,8 +34,6 @@
 // Common utilities.
 // -----------------------------------------------------------------------------
 
-const char* kDescriptorInstanceVar = "descriptor";
-
 static const char* get_str(VALUE str) {
   Check_Type(str, T_STRING);
   return RSTRING_PTR(str);
@@ -1590,9 +1588,9 @@ VALUE Builder_add_message(VALUE _self, VALUE name) {
  * call-seq:
  *     Builder.add_enum(name, &block)
  *
- * Creates a new, empty enum descriptor with the given name, and invokes the block in
- * the context of an EnumBuilderContext on that descriptor. The block can then
- * call EnumBuilderContext#add_value to define the enum values.
+ * Creates a new, empty enum descriptor with the given name, and invokes the
+ * block in the context of an EnumBuilderContext on that descriptor. The block
+ * can then call EnumBuilderContext#add_value to define the enum values.
  *
  * This is the recommended, idiomatic way to build enum definitions.
  */

ruby/ext/google/protobuf_c/encode_decode.c

@@ -644,7 +644,8 @@ static bool env_error_func(void* ud, const upb_status* status) {
   // Free the env -- rb_raise will longjmp up the stack past the encode/decode
   // function so it would not otherwise have been freed.
   stackenv_uninit(se);
-  rb_raise(rb_eRuntimeError, se->ruby_error_template, upb_status_errmsg(status));
+  rb_raise(rb_eRuntimeError, se->ruby_error_template,
+           upb_status_errmsg(status));
   // Never reached: rb_raise() always longjmp()s up the stack, past all of our
   // code, back to Ruby.
   return false;
@@ -673,7 +674,7 @@ static void stackenv_uninit(stackenv* se) {
  * and returns a message object with the corresponding field values.
  */
 VALUE Message_decode(VALUE klass, VALUE data) {
-  VALUE descriptor = rb_iv_get(klass, kDescriptorInstanceVar);
+  VALUE descriptor = rb_ivar_get(klass, descriptor_instancevar_interned);
   Descriptor* desc = ruby_to_Descriptor(descriptor);
   VALUE msgklass = Descriptor_msgclass(descriptor);
 
@@ -711,7 +712,7 @@ VALUE Message_decode(VALUE klass, VALUE data) {
  * and returns a message object with the corresponding field values.
  */
 VALUE Message_decode_json(VALUE klass, VALUE data) {
-  VALUE descriptor = rb_iv_get(klass, kDescriptorInstanceVar);
+  VALUE descriptor = rb_ivar_get(klass, descriptor_instancevar_interned);
   Descriptor* desc = ruby_to_Descriptor(descriptor);
   VALUE msgklass = Descriptor_msgclass(descriptor);
 
@@ -846,7 +847,7 @@ static void putsubmsg(VALUE submsg, const upb_fielddef *f, upb_sink *sink,
   if (submsg == Qnil) return;
 
   upb_sink subsink;
-  VALUE descriptor = rb_iv_get(submsg, kDescriptorInstanceVar);
+  VALUE descriptor = rb_ivar_get(submsg, descriptor_instancevar_interned);
   Descriptor* subdesc = ruby_to_Descriptor(descriptor);
 
   upb_sink_startsubmsg(sink, getsel(f, UPB_HANDLER_STARTSUBMSG), &subsink);
@@ -968,7 +969,8 @@ static void putmap(VALUE map, const upb_fielddef *f, upb_sink *sink,
     VALUE value = Map_iter_value(&it);
 
     upb_sink entry_sink;
-    upb_sink_startsubmsg(&subsink, getsel(f, UPB_HANDLER_STARTSUBMSG), &entry_sink);
+    upb_sink_startsubmsg(&subsink, getsel(f, UPB_HANDLER_STARTSUBMSG),
+                         &entry_sink);
     upb_sink_startmsg(&entry_sink);
 
     put_ruby_value(key, key_field, Qnil, depth + 1, &entry_sink);
@@ -1098,7 +1100,7 @@ static const upb_handlers* msgdef_json_serialize_handlers(Descriptor* desc) {
  * wire format.
  */
 VALUE Message_encode(VALUE klass, VALUE msg_rb) {
-  VALUE descriptor = rb_iv_get(klass, kDescriptorInstanceVar);
+  VALUE descriptor = rb_ivar_get(klass, descriptor_instancevar_interned);
   Descriptor* desc = ruby_to_Descriptor(descriptor);
 
   stringsink sink;
@@ -1129,7 +1131,7 @@ VALUE Message_encode(VALUE klass, VALUE msg_rb) {
  * Encodes the given message object into its serialized JSON representation.
  */
 VALUE Message_encode_json(VALUE klass, VALUE msg_rb) {
-  VALUE descriptor = rb_iv_get(klass, kDescriptorInstanceVar);
+  VALUE descriptor = rb_ivar_get(klass, descriptor_instancevar_interned);
   Descriptor* desc = ruby_to_Descriptor(descriptor);
 
   stringsink sink;

ruby/ext/google/protobuf_c/message.c

@@ -53,7 +53,7 @@ rb_data_type_t Message_type = {
 };
 
 VALUE Message_alloc(VALUE klass) {
-  VALUE descriptor = rb_iv_get(klass, kDescriptorInstanceVar);
+  VALUE descriptor = rb_ivar_get(klass, descriptor_instancevar_interned);
   Descriptor* desc = ruby_to_Descriptor(descriptor);
   MessageHeader* msg = (MessageHeader*)ALLOC_N(
       uint8_t, sizeof(MessageHeader) + desc->layout->size);
@@ -63,7 +63,7 @@ VALUE Message_alloc(VALUE klass) {
   // a collection happens during object creation in layout_init().
   VALUE ret = TypedData_Wrap_Struct(klass, &Message_type, msg);
   msg->descriptor = desc;
-  rb_iv_set(ret, kDescriptorInstanceVar, descriptor);
+  rb_ivar_set(ret, descriptor_instancevar_interned, descriptor);
 
   layout_init(desc->layout, Message_data(msg));
 
@@ -341,7 +341,8 @@ VALUE Message_to_h(VALUE _self) {
        !upb_msg_field_done(&it);
        upb_msg_field_next(&it)) {
     const upb_fielddef* field = upb_msg_iter_field(&it);
-    VALUE msg_value = layout_get(self->descriptor->layout, Message_data(self), field);
+    VALUE msg_value = layout_get(self->descriptor->layout, Message_data(self),
+                                 field);
     VALUE msg_key   = ID2SYM(rb_intern(upb_fielddef_name(field)));
     if (upb_fielddef_label(field) == UPB_LABEL_REPEATED) {
       msg_value = RepeatedField_to_ary(msg_value);
@@ -400,7 +401,7 @@ VALUE Message_index_set(VALUE _self, VALUE field_name, VALUE value) {
  * message class's type.
  */
 VALUE Message_descriptor(VALUE klass) {
-  return rb_iv_get(klass, kDescriptorInstanceVar);
+  return rb_ivar_get(klass, descriptor_instancevar_interned);
 }
 
 VALUE build_class_from_descriptor(Descriptor* desc) {
@@ -421,11 +422,13 @@ VALUE build_class_from_descriptor(Descriptor* desc) {
       // their own toplevel constant class name.
       rb_intern("Message"),
       rb_cObject);
-  rb_iv_set(klass, kDescriptorInstanceVar, get_def_obj(desc->msgdef));
+  rb_ivar_set(klass, descriptor_instancevar_interned,
+              get_def_obj(desc->msgdef));
   rb_define_alloc_func(klass, Message_alloc);
   rb_require("google/protobuf/message_exts");
   rb_include_module(klass, rb_eval_string("Google::Protobuf::MessageExts"));
-  rb_extend_object(klass, rb_eval_string("Google::Protobuf::MessageExts::ClassMethods"));
+  rb_extend_object(
+      klass, rb_eval_string("Google::Protobuf::MessageExts::ClassMethods"));
 
   rb_define_method(klass, "method_missing",
                    Message_method_missing, -1);
@@ -458,7 +461,7 @@ VALUE build_class_from_descriptor(Descriptor* desc) {
  */
 VALUE enum_lookup(VALUE self, VALUE number) {
   int32_t num = NUM2INT(number);
-  VALUE desc = rb_iv_get(self, kDescriptorInstanceVar);
+  VALUE desc = rb_ivar_get(self, descriptor_instancevar_interned);
   EnumDescriptor* enumdesc = ruby_to_EnumDescriptor(desc);
 
   const char* name = upb_enumdef_iton(enumdesc->enumdef, num);
@@ -478,7 +481,7 @@ VALUE enum_lookup(VALUE self, VALUE number) {
  */
 VALUE enum_resolve(VALUE self, VALUE sym) {
   const char* name = rb_id2name(SYM2ID(sym));
-  VALUE desc = rb_iv_get(self, kDescriptorInstanceVar);
+  VALUE desc = rb_ivar_get(self, descriptor_instancevar_interned);
   EnumDescriptor* enumdesc = ruby_to_EnumDescriptor(desc);
 
   int32_t num = 0;
@@ -498,7 +501,7 @@ VALUE enum_resolve(VALUE self, VALUE sym) {
  * EnumDescriptor corresponding to this enum type.
  */
 VALUE enum_descriptor(VALUE self) {
-  return rb_iv_get(self, kDescriptorInstanceVar);
+  return rb_ivar_get(self, descriptor_instancevar_interned);
 }
 
 VALUE build_module_from_enumdesc(EnumDescriptor* enumdesc) {
@@ -523,7 +526,8 @@ VALUE build_module_from_enumdesc(EnumDescriptor* enumdesc) {
   rb_define_singleton_method(mod, "lookup", enum_lookup, 1);
   rb_define_singleton_method(mod, "resolve", enum_resolve, 1);
   rb_define_singleton_method(mod, "descriptor", enum_descriptor, 0);
-  rb_iv_set(mod, kDescriptorInstanceVar, get_def_obj(enumdesc->enumdef));
+  rb_ivar_set(mod, descriptor_instancevar_interned,
+              get_def_obj(enumdesc->enumdef));
 
   return mod;
 }

ruby/ext/google/protobuf_c/protobuf.c

@@ -64,6 +64,15 @@ rb_encoding* kRubyStringUtf8Encoding;
 rb_encoding* kRubyStringASCIIEncoding;
 rb_encoding* kRubyString8bitEncoding;
 
+// Ruby-interned string: "descriptor". We use this identifier to store an
+// instance variable on message classes we create in order to link them back to
+// their descriptors.
+//
+// We intern this once at module load time then use the interned identifier at
+// runtime in order to avoid the cost of repeatedly interning in hot paths.
+const char* kDescriptorInstanceVar = "descriptor";
+ID descriptor_instancevar_interned;
+
 // -----------------------------------------------------------------------------
 // Initialization/entry point.
 // -----------------------------------------------------------------------------
@@ -71,6 +80,7 @@ rb_encoding* kRubyString8bitEncoding;
 // This must be named "Init_protobuf_c" because the Ruby module is named
 // "protobuf_c" -- the VM looks for this symbol in our .so.
 void Init_protobuf_c() {
+  descriptor_instancevar_interned = rb_intern(kDescriptorInstanceVar);
   VALUE google = rb_define_module("Google");
   VALUE protobuf = rb_define_module_under(google, "Protobuf");
   VALUE internal = rb_define_module_under(protobuf, "Internal");

ruby/ext/google/protobuf_c/protobuf.h

@@ -161,8 +161,6 @@ extern VALUE cOneofBuilderContext;
 extern VALUE cEnumBuilderContext;
 extern VALUE cBuilder;
 
-extern const char* kDescriptorInstanceVar;
-
 // We forward-declare all of the Ruby method implementations here because we
 // sometimes call the methods directly across .c files, rather than going
 // through Ruby's method dispatching (e.g. during message parse). It's cleaner
@@ -530,4 +528,6 @@ void check_upb_status(const upb_status* status, const char* msg);
     check_upb_status(&status, msg);                                           \
 } while (0)
 
+extern ID descriptor_instancevar_interned;
+
 #endif  // __GOOGLE_PROTOBUF_RUBY_PROTOBUF_H__

ruby/ext/google/protobuf_c/repeated_field.c

@@ -117,7 +117,8 @@ VALUE RepeatedField_index(int argc, VALUE* argv, VALUE _self) {
       if (index < 0 || index >= self->size) {
         return Qnil;
       }
-      void* memory = (void *) (((uint8_t *)self->elements) + index * element_size);
+      void* memory = (void *) (((uint8_t *)self->elements) +
+                               index * element_size);
       return native_slot_get(field_type, field_type_class, memory);
     }else{
       /* check if idx is Range */
@@ -497,13 +498,13 @@ VALUE RepeatedField_concat(VALUE _self, VALUE list) {
 
 
 void validate_type_class(upb_fieldtype_t type, VALUE klass) {
-  if (rb_iv_get(klass, kDescriptorInstanceVar) == Qnil) {
+  if (rb_ivar_get(klass, descriptor_instancevar_interned) == Qnil) {
     rb_raise(rb_eArgError,
              "Type class has no descriptor. Please pass a "
              "class or enum as returned by the DescriptorPool.");
   }
   if (type == UPB_TYPE_MESSAGE) {
-    VALUE desc = rb_iv_get(klass, kDescriptorInstanceVar);
+    VALUE desc = rb_ivar_get(klass, descriptor_instancevar_interned);
     if (!RB_TYPE_P(desc, T_DATA) || !RTYPEDDATA_P(desc) ||
         RTYPEDDATA_TYPE(desc) != &_Descriptor_type) {
       rb_raise(rb_eArgError, "Descriptor has an incorrect type.");
@@ -513,7 +514,7 @@ void validate_type_class(upb_fieldtype_t type, VALUE klass) {
                "Message class was not returned by the DescriptorPool.");
     }
   } else if (type == UPB_TYPE_ENUM) {
-    VALUE enumdesc = rb_iv_get(klass, kDescriptorInstanceVar);
+    VALUE enumdesc = rb_ivar_get(klass, descriptor_instancevar_interned);
     if (!RB_TYPE_P(enumdesc, T_DATA) || !RTYPEDDATA_P(enumdesc) ||
         RTYPEDDATA_TYPE(enumdesc) != &_EnumDescriptor_type) {
       rb_raise(rb_eArgError, "Descriptor has an incorrect type.");

ruby/ext/google/protobuf_c/storage.c

@@ -415,7 +415,8 @@ MessageLayout* create_layout(const upb_msgdef* msgdef) {
     // Align current offset up to |size| granularity.
     off = align_up_to(off, field_size);
     layout->fields[upb_fielddef_index(field)].offset = off;
-    layout->fields[upb_fielddef_index(field)].case_offset = MESSAGE_FIELD_NO_CASE;
+    layout->fields[upb_fielddef_index(field)].case_offset =
+        MESSAGE_FIELD_NO_CASE;
     off += field_size;
   }