-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Fewer allocated objects on each request #737
Conversation
👍 Yes please. |
👍 makes sense |
def respond_to?(*args) | ||
return false if args.first.to_s =~ /^to_ary$/ | ||
super or @body.respond_to?(*args) | ||
TO_ARY_REGEX = /^to_ary$/ |
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 was under impression that literal non-interpolated regexps such as /^to_ary$/
are singletons and don't actually allocate a Regexp
object on each execution.
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.
My favorite type of comment on an open source pull request: one that teaches me something i didn't know. You are correct. I tested this out with memory_profiler and funny enough, it doesn't even show a single object allocated
report = MemoryProfiler.report do
num.times {
foo = /^to_ary$/
}
end
report.pretty_print
Total allocated 0
Total retained 0
allocated memory by gem
-----------------------------------
allocated memory by file
-----------------------------------
allocated memory by location
-----------------------------------
allocated objects by gem
-----------------------------------
allocated objects by file
-----------------------------------
allocated objects by location
-----------------------------------
retained memory by gem
-----------------------------------
retained memory by file
-----------------------------------
retained memory by location
-----------------------------------
retained objects by gem
-----------------------------------
retained objects by file
-----------------------------------
retained objects by location
-----------------------------------
Allocated String Report
-----------------------------------
Retained String Report
-----------------------------------
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.
btw, I updated to move the regexp out of a constant and back into the method
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.
f226179
to
dc53a8c
Compare
Less allocated objects on each request
Thank you! <3 |
A big 👍 |
Nice :) |
def respond_to?(*args) | ||
return false if args.first.to_s =~ /^to_ary$/ | ||
super or @body.respond_to?(*args) | ||
def respond_to?(method_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.
In 1.9, respond_to takes 2 arguments.
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.
Right you are: http://ruby-doc.org/core-2.1.3/Object.html#method-i-respond_to-3F Looks like 1.8.7 & 2+ also take multiple arguments. I did this because the *args
causes us to have an extra array allocation. If we match the method signature respond_to?(string, include_all=false)
then the include_all
would be an extra object. If I remember this only decreased the overall count by around 100. I'm going to open a new PR to revert these changes, I don't think we can do better than the previous case and still maintain compatibility.
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.
Fix is here: #739 sorry about that, should have known better. Thanks for the catch!
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.
Sorry, but false.object_id == 0
, include_all in your example only takes a reference slot, not an RObject structure.
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 in #742 also can get rid of a string allocation in method_missing
The `respond_to?` method was optimized for less object creation, unfortunately I also mangled the method signature. This commit reverts the changes introduced in rack#737 to `BodyProxy#respond_to?` and adds tests. See: https://github.com/rack/rack/pull/737/files#r18359323 cc/ @raggi
@@ -12,13 +12,14 @@ def initialize(app, name = nil) | |||
@header_name << "-#{name}" if name | |||
end | |||
|
|||
FORMAT_STRING = "%0.6f" |
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.
Should be frozen?
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.
Yep, good catch. Thanks! Updated in #742
- 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
- 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
- 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
Nice Stuff! 👍 |
👍 |
- 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
- 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
How many? Using
memory_profiler
and a Rails app (codetriage.com), master uses:After this patch, the app uses:
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 boost 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.