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.

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

Abrandoned/decoding setters without copy #297

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
22 changes: 15 additions & 7 deletions lib/protobuf/field/base_field.rb
Original file line number Diff line number Diff line change
Expand Up @@ -176,16 +176,18 @@ def define_accessor
else
define_getter
define_setter
define_decode_setter
end
end

def define_array_getter
field = self
field_name = field.name
method_name = field.getter

message_class.class_eval do
define_method(method_name) do
@values[field.name] ||= ::Protobuf::Field::FieldArray.new(field)
@values[field_name] ||= ::Protobuf::Field::FieldArray.new(field)
Copy link
Contributor

Choose a reason for hiding this comment

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

Heads up that a lot of this work will need to be moved to the []= and [] methods after PR #302 is merged. In fact a lot of the setter/getter code in this PR will conflict, I comment on just a little of it below.

end
end

Expand All @@ -194,6 +196,7 @@ def define_array_getter

def define_array_setter
field = self
field_name = field.name
method_name = field.setter

message_class.class_eval do
Expand All @@ -205,15 +208,15 @@ def define_array_setter
else
fail TypeError, <<-TYPE_ERROR
Expected repeated value of type '#{field.type_class}'
Got '#{val.class}' for repeated protobuf field #{field.name}
Got '#{val.class}' for repeated protobuf field #{field_name}
TYPE_ERROR
end

if val.nil? || (val.respond_to?(:empty?) && val.empty?)
@values.delete(field.name)
@values.delete(field_name)
else
@values[field.name] ||= ::Protobuf::Field::FieldArray.new(field)
@values[field.name].replace(val)
@values[field_name] ||= ::Protobuf::Field::FieldArray.new(field)
@values[field_name].replace(val)
end
end
end
Expand All @@ -234,17 +237,22 @@ 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
field_name = field.name
method_name = field.setter

message_class.class_eval do
define_method(method_name) do |val|
@encode = nil
if val.nil? || (val.respond_to?(:empty?) && val.empty?)
@values.delete(field.name)
@values.delete(field_name)
elsif field.acceptable?(val)
@values[field.name] = field.coerce!(val)
@values[field_name] = field.coerce!(val)
else
fail TypeError, "Unacceptable value #{val} for field #{field.name} of type #{field.type_class}"
end
Expand Down
13 changes: 13 additions & 0 deletions lib/protobuf/field/bool_field.rb
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,19 @@ def decode(value)
value == 1
end

def define_decode_setter
field = self
field_name = 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(tag_method_name) do |val|
@encode = nil
Copy link
Contributor

Choose a reason for hiding this comment

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

I think @encode has gone away, from what @film42 has commented.

@values[field_name] = field.decode(val)
end
end
end

def encode(value)
[value ? 1 : 0].pack('C')
end
Expand Down
23 changes: 19 additions & 4 deletions lib/protobuf/field/bytes_field.rb
Original file line number Diff line number Diff line change
Expand Up @@ -56,22 +56,37 @@ def wire_type
# Private Instance Methods
#

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

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

def define_setter
field = self
field_name = field.name
field_type_class = field.type_class
method_name = field.setter

message_class.class_eval do
define_method(method_name) do |val|
@encode = nil
case val
when String, Symbol
@values[field.name] = "#{val}"
@values[field_name] = "#{val}"
when NilClass
@values.delete(field.name)
@values.delete(field_name)
when ::Protobuf::Message
@values[field.name] = val.dup
@values[field_name] = val.dup
else
fail TypeError, "Unacceptable value #{val} for field #{field.name} of type #{field.type_class}"
fail TypeError, "Unacceptable value #{val} for field #{field_name} of type #{field_type_class}"
end
end
end
Expand Down
25 changes: 21 additions & 4 deletions lib/protobuf/field/enum_field.rb
Original file line number Diff line number Diff line change
Expand Up @@ -38,19 +38,36 @@ def enum?
# Private Instance Methods
#

def define_decode_setter
field = self
field_name = field.name
field_type_class = field.type_class
tag_method_name = "_protobuf_decode_setter_#{field.tag}"

message_class.class_eval do
define_method(tag_method_name) do |val|
@encode = nil
@values[field_name] = field_type_class.fetch(val)
end
end
end

def define_setter
field = self
field_name = field.name
field_type_class = field.type_class

message_class.class_eval do
define_method("#{field.name}=") do |value|
define_method("#{field_name}=") do |value|
@encode = nil
orig_value = value
if value.nil?
@values.delete(field.name)
@values.delete(field_name)
else
value = field.type_class.fetch(value)
value = field_type_class.fetch(value)
fail TypeError, "Invalid Enum value: #{orig_value.inspect} for #{field.name}" unless value

@values[field.name] = value
@values[field_name] = value
end
end
end
Expand Down
15 changes: 14 additions & 1 deletion lib/protobuf/field/integer_field.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,23 @@ class IntegerField < VarintField
#

def decode(value)
value -= 0x1_0000_0000_0000_0000 if (value & 0x8000_0000_0000_0000).nonzero?
value -= 0x1_0000_0000_0000_0000 if (value & 0x8000_0000_0000_0000) != 0
value
end

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

message_class.class_eval do
define_method(tag_method_name) do |val|
@encode = nil
@values[field_name] = field.decode(val)
end
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
21 changes: 11 additions & 10 deletions lib/protobuf/field/message_field.rb
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 All @@ -38,20 +36,23 @@ def wire_type

def define_setter
field = self
field_name = field.name
field_type_class = field.type_class

message_class.class_eval do
define_method("#{field.name}=") do |val|
define_method("#{field_name}=") do |val|
@encode = nil
case
when val.nil?
@values.delete(field.name)
when val.is_a?(field.type_class)
@values[field.name] = val
@values.delete(field_name)
when val.is_a?(field_type_class)
@values[field_name] = val
when val.respond_to?(:to_proto)
@values[field.name] = val.to_proto
@values[field_name] = val.to_proto
when val.respond_to?(:to_hash)
@values[field.name] = field.type_class.new(val.to_hash)
@values[field_name] = field_type_class.new(val.to_hash)
else
fail TypeError, "Expected value of type '#{field.type_class}' for field #{field.name}, but got '#{val.class}'"
fail TypeError, "Expected value of type '#{field_type_class}' for field #{field_name}, but got '#{val.class}'"
end
end
end
Expand Down
2 changes: 1 addition & 1 deletion lib/protobuf/field/sfixed32_field.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ class Sfixed32Field < Int32Field

def decode(bytes)
value = bytes.unpack('V').first
value -= 0x1_0000_0000 if (value & 0x8000_0000).nonzero?
value -= 0x1_0000_0000 if (value & 0x8000_0000) != 0
value
end

Expand Down
2 changes: 1 addition & 1 deletion lib/protobuf/field/sfixed64_field.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ class Sfixed64Field < Int64Field
def decode(bytes)
values = bytes.unpack('VV') # 'Q' is machine-dependent, don't use
value = values[0] + (values[1] << 32)
value -= 0x1_0000_0000_0000_0000 if (value & 0x8000_0000_0000_0000).nonzero?
value -= 0x1_0000_0000_0000_0000 if (value & 0x8000_0000_0000_0000) != 0
value
end

Expand Down
13 changes: 13 additions & 0 deletions lib/protobuf/field/signed_integer_field.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,19 @@ def decode(value)
end
end

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

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

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

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

message_class.class_eval do
define_method(tag_method_name) do |val|
@encode = nil
val.force_encoding(::Protobuf::Field::StringField::ENCODING)
@values[field_name] = val
end
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
14 changes: 14 additions & 0 deletions lib/protobuf/field/varint_field.rb
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,19 @@ def coerce!(val)
Integer(val, 10)
end

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

message_class.class_eval do
define_method(tag_method_name) do |val|
@encode = nil
@values[field_name] = val
end
end
end

def decode(value)
value
end
Expand Down
19 changes: 8 additions & 11 deletions lib/protobuf/message.rb
Original file line number Diff line number Diff line change
Expand Up @@ -77,14 +77,13 @@ 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

def each_field_for_serialization
self.class.all_fields.each do |field|
value = @values[field.getter]
value = @values[field.name]
if value.nil?
fail ::Protobuf::SerializationError, "Required field #{self.class.name}##{field.name} does not have a value." if field.required?
next
Expand Down Expand Up @@ -146,18 +145,16 @@ def ==(other)
end

def [](name)
if (field = self.class.get_field(name, true))
__send__(field.getter)
end
__send__(name) if respond_to?(name)
end

def []=(name, value)
if (field = self.class.get_field(name, true))
__send__(field.setter, value) unless value.nil?
setter = "#{name}="

if respond_to?(setter)
__send__(setter, value) unless value.nil?
else
unless ::Protobuf.ignore_unknown_fields?
fail ::Protobuf::FieldNotDefinedError, name
end
fail ::Protobuf::FieldNotDefinedError, name unless ::Protobuf.ignore_unknown_fields?
end
end

Expand Down
3 changes: 3 additions & 0 deletions lib/protobuf/message/serialization.rb
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
8 changes: 4 additions & 4 deletions lib/protobuf/varint_pure.rb
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
module Protobuf
module VarintPure
def decode(stream)
value = index = 0
value = shift = 0
begin
byte = stream.readbyte
value |= (byte & 0x7f) << (7 * index)
index += 1
end while (byte & 0x80).nonzero?
value |= (byte & 127) << shift
Copy link
Contributor

Choose a reason for hiding this comment

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

Bikeshedding a little, but honestly I prefer the hex. Hex makes it easier to picture the individual bits in my head.

shift += 7
end while byte > 127
value
end
end
Expand Down
Loading