From 231886f6327b7cad480b96d08595f45b3526cb4a Mon Sep 17 00:00:00 2001 From: Chris Fallin Date: Tue, 19 May 2015 15:33:48 -0700 Subject: Ruby C extension speedup: don't re-intern constant string needlessly. Also fixed lines with > 80 char length. --- ruby/ext/google/protobuf_c/defs.c | 8 +++----- ruby/ext/google/protobuf_c/encode_decode.c | 16 +++++++++------- ruby/ext/google/protobuf_c/message.c | 24 ++++++++++++++---------- ruby/ext/google/protobuf_c/protobuf.c | 10 ++++++++++ ruby/ext/google/protobuf_c/protobuf.h | 4 ++-- ruby/ext/google/protobuf_c/repeated_field.c | 9 +++++---- ruby/ext/google/protobuf_c/storage.c | 3 ++- 7 files changed, 45 insertions(+), 29 deletions(-) (limited to 'ruby/ext') diff --git a/ruby/ext/google/protobuf_c/defs.c b/ruby/ext/google/protobuf_c/defs.c index 9b590a55..e3341cca 100644 --- a/ruby/ext/google/protobuf_c/defs.c +++ b/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. */ diff --git a/ruby/ext/google/protobuf_c/encode_decode.c b/ruby/ext/google/protobuf_c/encode_decode.c index 736f573e..fe249d45 100644 --- a/ruby/ext/google/protobuf_c/encode_decode.c +++ b/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; diff --git a/ruby/ext/google/protobuf_c/message.c b/ruby/ext/google/protobuf_c/message.c index 55a674cf..4be1c8c8 100644 --- a/ruby/ext/google/protobuf_c/message.c +++ b/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; } diff --git a/ruby/ext/google/protobuf_c/protobuf.c b/ruby/ext/google/protobuf_c/protobuf.c index 8ab518a5..d0625a10 100644 --- a/ruby/ext/google/protobuf_c/protobuf.c +++ b/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"); diff --git a/ruby/ext/google/protobuf_c/protobuf.h b/ruby/ext/google/protobuf_c/protobuf.h index 3d619617..f8667486 100644 --- a/ruby/ext/google/protobuf_c/protobuf.h +++ b/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__ diff --git a/ruby/ext/google/protobuf_c/repeated_field.c b/ruby/ext/google/protobuf_c/repeated_field.c index dc1d0355..ffa60c15 100644 --- a/ruby/ext/google/protobuf_c/repeated_field.c +++ b/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."); diff --git a/ruby/ext/google/protobuf_c/storage.c b/ruby/ext/google/protobuf_c/storage.c index 2ad8bd74..c4cdc9cc 100644 --- a/ruby/ext/google/protobuf_c/storage.c +++ b/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; } -- cgit v1.2.3