-
Notifications
You must be signed in to change notification settings - Fork 100
Small optimizations #308
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
Small optimizations #308
Conversation
begin | ||
byte = stream.readbyte | ||
value |= (byte & 0x7f) << (7 * index) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious, is the hex actually slower than the dec?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They're about the same, but hex is slower:
irb(main):002:0> Benchmark.ips do |x|
irb(main):003:1* x.report("hex") { 3 + 0x7F }
irb(main):004:1> x.report("dec") { 3 + 127 }
irb(main):005:1> end
Calculating -------------------------------------
hex 130.033k i/100ms
dec 119.055k i/100ms
-------------------------------------------------
hex 8.761M (± 4.6%) i/s - 43.691M
dec 9.034M (± 5.7%) i/s - 45.003M
If you think it reads better as a hex (fine either way for me; google uses both), I'll change it back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems pretty variable:
Warming up --------------------------------------
hex 186.594k i/100ms
dec 195.158k i/100ms
Calculating -------------------------------------
hex 9.104M (± 6.8%) i/s - 45.342M
dec 8.828M (± 6.4%) i/s - 44.106M
But I don't want to nitpick, either way is fine :). Hex is nicer for visualizing bitwise arithmetic, but I'm was mostly just curious :)
👍 seems good, though I prefer the hex unless it's slower :) |
Seems legit, LGTM 👍 |
👍 this looks good to me. No strong preference on |
end | ||
|
||
def #{simple_field_name}=(value) | ||
self[:#{fully_qualified_field_name}] = value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we know that fully_qualified_field_name
will be a string and not a symbol?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Symbol or string, it should be the same: puts :ok #=> "ok"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, maybe it should look like :"#{fully_qualified_field_name}"
to support weird symbols like :"some.sym.yolo"
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spec to verify?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated. Now that the full qualified field name code is in master, I rebased to see how the current implementation would work and got errors because :some.thing
is not valid ruby. Change this to :"some.thing"
works just fine.
This optimization is slightly faster.
Without this, specs won't even run.
@values[fully_qualified_field_name] | ||
message_class.class_eval <<-ruby, __FILE__, __LINE__ | ||
def #{simple_field_name}! | ||
@values[:"#{fully_qualified_field_name}"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could also do @values[#{fully_qualified_field_name.inspect}]
, but I don't really have a preference.
@film42 were we going to pull this in? looks like it needs to be updated if so |
Still planning on it, but working through getting 3.7.0 features into master first, then coming back and doing a full profile again. |
This just adds a few speedups.
class_eval
string - This turned out to be about 40% faster for setters and 48% faster for getters (for both MRI and Jruby). Here's my benchmark: https://gist.github.com/film42/8efd4810d3c5a5e12291.nonzero?
vs!= 0
- Declarative methods are nice, but simple operators are still very readable and more performant, especially in these hot code paths.Note: I also added
benchmark-ips
as a dev dependency because I end up using it a lot when working with protobuf.cc @abrandoned @zachmargolis @embark