Skip to content

Commit

Permalink
Remove memoization on encode
Browse files Browse the repository at this point in the history
Changing values in repeated fields does not clear the memoized encoded
value. This means that changes to repated fields after the memoized
encoding has been computed will never be encoded. The pattern works for
other field types because the memoized value is being cleared in the
setter on the message before we call into the field. The field itself is
not involved in clearing the memoized value at all.

Repeated fields are a different story. If we were completely resetting a
repeated field to a new array, the memoized encoding would be correctly
cleared, but because the field array we are pushing into doesn't have any
concept of a message, let alone the specific message instance the field
belongs to, there is no way to clear the memoized encoded message.

We need to remove the memoization until we can find a better approach.

Fixes #304.
liveh2o committed Feb 18, 2016
1 parent 2970c0d commit ad4d852
Showing 6 changed files with 4 additions and 69 deletions.
2 changes: 0 additions & 2 deletions lib/protobuf/field/base_field.rb
Original file line number Diff line number Diff line change
@@ -198,7 +198,6 @@ def define_array_setter

message_class.class_eval do
define_method(method_name) do |val|
@encode = nil
if val.is_a?(Array)
val = val.dup
val.compact!
@@ -240,7 +239,6 @@ def define_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)
elsif field.acceptable?(val)
1 change: 0 additions & 1 deletion lib/protobuf/field/bytes_field.rb
Original file line number Diff line number Diff line change
@@ -62,7 +62,6 @@ def define_setter

message_class.class_eval do
define_method(method_name) do |val|
@encode = nil
case val
when String, Symbol
@values[field.name] = "#{val}"
1 change: 0 additions & 1 deletion lib/protobuf/field/enum_field.rb
Original file line number Diff line number Diff line change
@@ -42,7 +42,6 @@ def define_setter
field = self
message_class.class_eval do
define_method("#{field.name}=") do |value|
@encode = nil
orig_value = value
if value.nil?
@values.delete(field.name)
1 change: 0 additions & 1 deletion lib/protobuf/field/message_field.rb
Original file line number Diff line number Diff line change
@@ -40,7 +40,6 @@ def define_setter
field = self
message_class.class_eval do
define_method("#{field.name}=") do |val|
@encode = nil
case
when val.nil?
@values.delete(field.name)
10 changes: 4 additions & 6 deletions lib/protobuf/message/serialization.rb
Original file line number Diff line number Diff line change
@@ -48,12 +48,10 @@ def decode_from(stream)
# Encode this message
#
def encode
@encode ||= begin
stream = ::StringIO.new
stream.set_encoding(::Protobuf::Field::BytesField::BYTES_ENCODING)
encode_to(stream)
stream.string
end
stream = ::StringIO.new
stream.set_encoding(::Protobuf::Field::BytesField::BYTES_ENCODING)
encode_to(stream)
stream.string
end

# Encode this message to the given stream.
58 changes: 0 additions & 58 deletions spec/lib/protobuf/message_spec.rb
Original file line number Diff line number Diff line change
@@ -268,64 +268,6 @@
end
end

describe 'memoization' do
it "should memoize enum message" do
test_enum = Test::EnumTestMessage.new
test_enum.encode
expect(test_enum.instance_variable_get(:@encode)).to eq("")
test_enum.non_default_enum = 2
expect(test_enum.instance_variable_get(:@encode)).to be_nil
end

context "boolean fields" do
let(:values) { { :ext_is_searchable => true, :name => "STEPH CURRY" } }
let(:test_resource) { ::Test::Resource.new(values) }

it "should memoize after bool values change " do
test_resource.encode
expect(test_resource.instance_variable_get(:@encode)).to eq(test_resource.encode)
test_resource.ext_is_searchable = false
expect(test_resource.instance_variable_get(:@encode)).to be_nil
end
end

context "string" do
let(:values) { { :ext_is_searchable => true, :name => "STEPH CURRY" } }
let(:test_resource) { ::Test::Resource.new(values) }

it "should memoize after bool values change " do
test_resource.encode
expect(test_resource.instance_variable_get(:@encode)).to eq(test_resource.encode)
test_resource.name = "MVP"
expect(test_resource.instance_variable_get(:@encode)).to be_nil
end
end

context "string" do
let(:values) { { :ext_is_searchable => true, :name => "STEPH CURRY" } }
let(:test_resource) { ::Test::Resource.new(values) }

it "should memoize after string values change " do
test_resource.encode
expect(test_resource.instance_variable_get(:@encode)).to eq(test_resource.encode)
test_resource.name = "MVP"
expect(test_resource.instance_variable_get(:@encode)).to be_nil
end
end

context "Int64" do
let(:values) { { :name => "STEPH CURRY", :date_created => 1454712125 } }
let(:test_resource) { ::Test::Resource.new(values) }

it "should memoize after Int64 values change " do
test_resource.encode
expect(test_resource.instance_variable_get(:@encode)).to eq(test_resource.encode)
test_resource.date_created = 5554712127
expect(test_resource.instance_variable_get(:@encode)).to be_nil
end
end
end

context "when there's no value for a required field" do
let(:message) { ::Test::ResourceWithRequiredField.new }

0 comments on commit ad4d852

Please sign in to comment.