Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Abrandoned/decoding setters without copy #297

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Abrandoned/decoding setters without copy #297

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
22909fb
add a set of decoding setters for each type that removes the extra co…
abrandoned Feb 8, 2016
7637d60
account for varint types that nee to run through decode and check if …
abrandoned Feb 8, 2016
e06f642
do not need to go through field to get the field.name when running th…
abrandoned Feb 8, 2016
d096a27
take out nonzero and rewrite VarintPure and use GraphProfile for Jruby
abrandoned Feb 8, 2016
0a9bfbb
Merge branch 'master' into abrandoned/decoding_setters_without_copy
abrandoned Feb 8, 2016
481e730
cache type_class when used and make sure we set a valid enum on sette…
abrandoned Feb 9, 2016
476844f
remove the name setter for decode as tag is only one needed
abrandoned Feb 9, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions lib/protobuf/field/base_field.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,7 @@ def define_accessor
else
define_getter
define_setter
define_decode_setter
end
end

Expand Down Expand Up @@ -234,6 +235,10 @@ def define_getter
::Protobuf.field_deprecator.deprecate_method(message_class, method_name) if field.deprecated?
end

def define_decode_setter
# empty for now
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only a few variants of this method. Let's bring the duplicated common implementation here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably best, I wrote this while I was testing the string copy only and now need to refactor since a common pattern has emerged

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

end

def define_setter
field = self
method_name = field.setter
Expand Down
14 changes: 14 additions & 0 deletions lib/protobuf/field/bool_field.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,20 @@ def decode(value)
value == 1
end

def define_decode_setter
field = self
name_method_name = "_protobuf_decode_setter_#{field.name}"
tag_method_name = "_protobuf_decode_setter_#{field.tag}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make this private, message_class.instance_eval { private tag_method_name.to_sym } or some such?


message_class.class_eval do
define_method(name_method_name) do |val|
@values[field.name] = field.decode(val)
end

alias_method tag_method_name, name_method_name
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in lib/protobuf/message/serialization.rb looks like we only ever call the tag version of this method, why even define the field name versions at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can take out the name versions; I was originally just looking for a tag version so decoding doesn't need to lookup the field; will change this

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, I think that this will help avoid some extension vs field issues later down the line as well, since tags are 100% unambiguous

end
end

def encode(value)
[value ? 1 : 0].pack('C')
end
Expand Down
15 changes: 15 additions & 0 deletions lib/protobuf/field/bytes_field.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,21 @@ def wire_type
# Private Instance Methods
#

def define_decode_setter
field = self
name_method_name = "_protobuf_decode_setter_#{field.name}"
tag_method_name = "_protobuf_decode_setter_#{field.tag}"

message_class.class_eval do
define_method(name_method_name) do |val|
@encode = nil
@values[field.name] = field.decode(val)
end

alias_method tag_method_name, name_method_name
end
end

def define_setter
field = self
method_name = field.setter
Expand Down
14 changes: 14 additions & 0 deletions lib/protobuf/field/enum_field.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,20 @@ def enum?
# Private Instance Methods
#

def define_decode_setter
field = self
name_method_name = "_protobuf_decode_setter_#{field.name}"
tag_method_name = "_protobuf_decode_setter_#{field.tag}"

message_class.class_eval do
define_method(name_method_name) do |val|
@values[field.name] = field.decode(val)
end

alias_method tag_method_name, name_method_name
end
end

def define_setter
field = self
message_class.class_eval do
Expand Down
14 changes: 14 additions & 0 deletions lib/protobuf/field/integer_field.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,20 @@ def decode(value)
value
end

def define_decode_setter
field = self
name_method_name = "_protobuf_decode_setter_#{field.name}"
tag_method_name = "_protobuf_decode_setter_#{field.tag}"

message_class.class_eval do
define_method(name_method_name) do |val|
@values[field.name] = field.decode(val)
end

alias_method tag_method_name, name_method_name
end
end

def encode(value)
# original Google's library uses 64bits integer for negative value
::Protobuf::Field::VarintField.encode(value & 0xffff_ffff_ffff_ffff)
Expand Down
4 changes: 1 addition & 3 deletions lib/protobuf/field/message_field.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,7 @@ def decode(bytes)
end

def encode(value)
bytes = value.encode
result = ::Protobuf::Field::VarintField.encode(bytes.size)
result << bytes
"#{::Protobuf::Field::VarintField.encode(value.encode.size)}#{value.encode}"
end

def message?
Expand Down
14 changes: 14 additions & 0 deletions lib/protobuf/field/signed_integer_field.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,20 @@ def decode(value)
end
end

def define_decode_setter
field = self
name_method_name = "_protobuf_decode_setter_#{field.name}"
tag_method_name = "_protobuf_decode_setter_#{field.tag}"

message_class.class_eval do
define_method(name_method_name) do |val|
@values[field.name] = field.decode(val)
end

alias_method tag_method_name, name_method_name
end
end

def encode(value)
if value >= 0
VarintField.encode(value << 1)
Expand Down
15 changes: 15 additions & 0 deletions lib/protobuf/field/string_field.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,21 @@ def decode(bytes)
bytes_to_decode
end

def define_decode_setter
field = self
name_method_name = "_protobuf_decode_setter_#{field.name}"
tag_method_name = "_protobuf_decode_setter_#{field.tag}"

message_class.class_eval do
define_method(name_method_name) do |val|
val.force_encoding(::Protobuf::Field::StringField::ENCODING)
@values[field.name] = val
end

alias_method tag_method_name, name_method_name
end
end

def encode(value)
value_to_encode = value.dup
value_to_encode.encode!(::Protobuf::Field::StringField::ENCODING, :invalid => :replace, :undef => :replace, :replace => "")
Expand Down
15 changes: 15 additions & 0 deletions lib/protobuf/field/varint_field.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ def self.encode(value, use_cache = true)
#

def acceptable?(val)
return true if val.is_a?(Integer) && val >= 0 && val < INT32_MAX
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we still want to compare to self.class.max right?

I understand wanting to avoid coerce! for performance reasons but maybe there's a better way to preserve the existing logic?

Just an idea

def acceptable?(val)
  int_val = val.is_a?(Integer)
    val
  else
    coerce!(val)
  end

  int_val >= self.class.min && int_val <= self.class.max
end

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we do want to compare the max (which happens after this if it is larger than the smallest max); the smallest max is the INT32_MAX, so if it passes it (and is an Integer; which is the most common case, especially in deserialization) then we just set it instead of taking the long route (which in our usage is very uncommon)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess as a reader of this patch, I'm having a hard following what it's supposed to do. I didn't realize it would fall through to the old case. Maybe we could do something like this (comments included) for posterity?

def acceptable?(val)
  int_val = if val.is_a?(Integer)
    return true if val >= 0 && val < INT32_MAX # return quickly for smallest integer size, hot code path
    val
  else
    coerce!(val)
  end
  int_val >= self.class.min && int_val <= self.class.max
end

int_val = coerce!(val)
int_val >= self.class.min && int_val <= self.class.max
rescue
Expand All @@ -63,6 +64,20 @@ def coerce!(val)
Integer(val, 10)
end

def define_decode_setter
field = self
name_method_name = "_protobuf_decode_setter_#{field.name}"
tag_method_name = "_protobuf_decode_setter_#{field.tag}"

message_class.class_eval do
define_method(name_method_name) do |val|
@values[field.name] = val
end

alias_method tag_method_name, name_method_name
end
end

def decode(value)
value
end
Expand Down
3 changes: 1 addition & 2 deletions lib/protobuf/message.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,7 @@ def each_field
return to_enum(:each_field) unless block_given?

self.class.all_fields.each do |field|
value = __send__(field.getter)
yield(field, value)
yield(field, __send__(field.getter))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After #302 it would be the style to use yield(field, self[field.name]), you'll probably get a merge conflict anyway, and in each_field_for_serialization it would probably be self[field.name] too.

end
end

Expand Down
3 changes: 3 additions & 0 deletions lib/protobuf/message/serialization.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,9 @@ def encode_to(stream)
private

def set_field_bytes(tag, bytes)
tag_setter_name = "_protobuf_decode_setter_#{tag}"
return __send__(tag_setter_name, bytes) if respond_to?(tag_setter_name)

field = self.class.get_field(tag, true)
field.set(self, bytes) if field
end
Expand Down