Conversation
| value = shift = 0 | ||
| begin | ||
| byte = stream.readbyte | ||
| value |= (byte & 0x7f) << (7 * index) |
There was a problem hiding this comment.
Curious, is the hex actually slower than the dec?
There was a problem hiding this comment.
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.
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 |
lib/protobuf/field/base_field.rb
Outdated
| end | ||
|
|
||
| def #{simple_field_name}=(value) | ||
| self[:#{fully_qualified_field_name}] = value |
There was a problem hiding this comment.
do we know that fully_qualified_field_name will be a string and not a symbol?
There was a problem hiding this comment.
Symbol or string, it should be the same: puts :ok #=> "ok"
There was a problem hiding this comment.
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.
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.
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. |
6 participants
This just adds a few speedups.
class_evalstring - 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-ipsas a dev dependency because I end up using it a lot when working with protobuf.cc @abrandoned @zachmargolis @embark