Missed optimizations - #742
Conversation
85a9929 to
9b922b5
Compare
9b922b5 to
751b8d5
Compare
There was a problem hiding this comment.
Isn't the code provided by @raggi #739 (comment) better?.
There was a problem hiding this comment.
this will do less work in the to_ary case, provided a symbol is supplied. A constant string of "to_ary" would be faster than the regex though. it's a mix of optimizations and lazy at this point.
There was a problem hiding this comment.
I like using case with a string constant, updated the code.
751b8d5 to
6c90af7
Compare
|
Also noticed that the keys in Rack::Mock are not using the proper case in |
|
Build is now passing |
- freezing constant string to ensure it's not mutated - use constant where available - optimize `respond_to?` to take less memory. Discussed in rack#737 and rack#739 `respond_to?` takes two arguments all recent rubies: - http://ruby-doc.org/core-2.1.3/Object.html#method-i-respond_to-3F - http://ruby-doc.org/core-1.9.3/Object.html#method-i-respond_to-3F - http://ruby-doc.org/core-1.8.7/Object.html#method-i-respond_to-3F Also `method_missing` will return a symbol from the first argument: - http://ruby-doc.org/core-2.1.3/BasicObject.html#method-i-method_missing - http://ruby-doc.org/core-1.9.3/BasicObject.html#method-i-method_missing - http://ruby-doc.org/core-1.8.7/Kernel.html#method-i-method_missing
6c90af7 to
5f808aa
Compare
|
Updated to use a string in the case statement instead of a constant based on: 68e086d#commitcomment-8050564 |
There was a problem hiding this comment.
simpler to write as
case method_name
when :to_ary, 'to_ary'
false
else
super or @body.respond_to?(method_name, include_all)
endAlso, if this isn't super hotspot, would be easier to write if method_name.to_s == 'to_ary'. If this is a super hotspot, can also extract TO_ARY = 'to_ary'.freeze, to avoid extra object allocation on each request.
There was a problem hiding this comment.
TO_ARY = 'to_ary'.freeze
Try this, in a case statement with and without. Ruby does this optimization for you, so it will actually be slower to use a constant. Read this thread https://groups.google.com/forum/#!topic/rack-devel/SKAE_yqN8-0
if method_name.to_s == 'to_ary'
This is slow, the reason is because of the to_s will allocate another string. The point of this PR in the first place was to get rid of that to_s
I appreciate you looking at this PR. Performance PRs take an extra long amount of time to put together due to all the benchmark running for proof. If you're suggesting a change for a performance reason, (even if it's to someone else's PR) please go the extra mile and benchmark your suggestion first. Here's all my benchmarks https://github.com/schneems/derailed_benchmarks this is at the application level. Most changes such as the one you suggested can be proved or disproved my making your own benchmark scripts in around 20 lines or less using benchmark/ips. Something to this: rails/rails#17250 .
|
I'd revert all these commits back to using string literals + in the cases where applies .freeze method. Favoring this way Ruby 2.1+ over old Ruby versions. In latests Rubies there are optimizations made when using .freeze, when using a string as a key of a Hash and also I think in 2.3 are coming optimizations for cases like /cc @raggi @jeremy please jump in and let's decide what to do. |
|
Ruby 2.2 isn't out yet and won't be officially stable till december. Even then there are people who can't upgrade immediately that will benefit from the extra speed. Even if we don't want to do constant frozen strings, we should still update the |
|
@schneems agreed about |
|
Lets be clear about speed here. In Rails this is faster because the GC is dominating your benchmarks. That's because rails is fat, and the heap is fragmenting during the benchmark (map it). If you do a micro-benchmark under pure rack, where head slabs are regularly rotated then temporary garbage is completely irrelevant to the benchmark numbers. In these cases, the cost of constant lookup is greater than the cost of string construction. I will not argue against a point that Rails accounts for most of our users, however, I'm still not 100% persuaded that Rails GC problems are (or should be) Rack core problems. |
|
If we really wanted speed here, over all else, Symbols are the way forward. Ruby-core still doesn't have a native HWIA though, so, until that exists, again I'm not sure that's a good choice for Rack. |
|
So after further thought, here's where I'm landing:
Given these trade-offs I'd say we can make a choice to preference assisting people with the large heap problems, so long as it's not a huge penalty for those with simple apps. It's a global effort optimization, rather than a local roflscale optimization ;-) |
|
I did some benchmarks with 2.2.0 There are string optimizations going into 2.2.0 but only for hash access and creation (for now) https://gist.github.com/schneems/698df49c8c22cfea1d2c Using a test Rails 4.2.0beta2 app I tested my previous commit and the current Ruby beta i'm seeing these differences: https://gist.github.com/schneems/df188395c862221071df We're still using less memory and there is still a speed up but neither are as dramatic. |
|
Another final note: as we transition into a multithreaded server world, the GC troubles will only get worse for all users, so in this case, going for the constants is also a good move. |
|
What did we decide? From my benchmarks it's still faster on 2.2. |
|
@schneems you never compared against |
How many? Using `memory_profiler` and a Rails app (codetriage.com), master uses: ``` rack/lib x 7318 ``` After this patch, the app uses: ``` rack/lib x 4598 ``` Or `(7318 - 4598) / 7318.0 * 100 # => 37.16` % fewer objects __PER REQUEST__. To do this, I extracted really commonly used strings into top level Rack constants. It makes for a bit of a big diff, but I believe the changes are worth it. Running benchmark/ips against the same app, I'm seeing a performance host of `2~4%` across the entire app response. This doesn't just make Rack faster, it will make your app faster. While we could certainly go overboard and pre-define ALL strings as constants, that would be pretty gnarly to work with. This patch goes after the largest of the low hanging fruit.
4 participants
respond_to?to not allocate array.Discussed in #737 and #739
respond_to?takes two arguments all recent rubies:Also
method_missingwill return a symbol from the first argument, we don't need to allocate a string:cc/ @raggi