Skip to content

Speed Up Creates & Updates By Memoizing on encode#293

Merged
abrandoned merged 12 commits intoruby-protobuf:masterfrom
vintagepenguin:dj/memoize-on-encode
Feb 6, 2016
Merged

Speed Up Creates & Updates By Memoizing on encode#293
abrandoned merged 12 commits intoruby-protobuf:masterfrom
vintagepenguin:dj/memoize-on-encode

Conversation

@vintagepenguin
Copy link
Copy Markdown

Serialization is one of the slowest things that you do in Protobuf. If we already have an encoded message, then we don't need to encode it again so why not memoize it.

@abrandoned @liveh2o @mmmries @brianstien


##
# Attributes
#
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why would we need the attr_accessor? (either of them)


message_class.class_eval do
define_method(method_name) do |val|
@memoized_encoded = nil
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we might want to say @has_change = true. It becomes more clear to me what we're trying to keep track of. That's just my opinion though.

@vintagepenguin
Copy link
Copy Markdown
Author

@film42 commenting here since the diff you commented on got deleted.

But yeah, I think @encode = nil it is confusing if you're looking at this for sure.

But the goal here it to clear out that instance variable so that when we hit the memoization in the encode method we know that we need to encode the message again because there has been a change and we know that because the setter was called.

@film42
Copy link
Copy Markdown
Member

film42 commented Feb 6, 2016

@vintagepenguin It would be more clear set a flag, or call a bust_encode_cache! method or something. But this seems 👍 .

Looks like you have a 👮 to fix, but then :shipit: .

abrandoned added a commit that referenced this pull request Feb 6, 2016
Speed Up Creates & Updates By Memoizing on encode
@abrandoned abrandoned merged commit f8228de into ruby-protobuf:master Feb 6, 2016
@claygoddard
Copy link
Copy Markdown

It seems that with this change, performing operations on repeated fields is no longer handled properly after the memoized encoding has been computed.

a.encode
a.some_repeated_field << "new-value"
a.encode # doesn't contain "new-value"

@liveh2o
Copy link
Copy Markdown
Contributor

liveh2o commented Feb 17, 2016

That's definitely a problem, @goddardc. Mind writing up an issue?

@claygoddard
Copy link
Copy Markdown

@liveh2o Sure thing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants