/rack

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

Less allocated objects on each request #737

Merged
merged 1 commit into from Oct 2, 2014

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
@schneems
Contributor

schneems commented Oct 1, 2014

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 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.
@nthj

This comment has been minimized.

Show comment
Hide comment
@nthj

nthj Oct 2, 2014

👍 Yes please.

nthj commented Oct 2, 2014

👍 Yes please.
@bbwharris

This comment has been minimized.

Show comment
Hide comment
@bbwharris

bbwharris Oct 2, 2014

👍 makes sense

bbwharris commented Oct 2, 2014

👍 makes sense
Show outdated Hide outdated lib/rack/body_proxy.rb
@schneems
Less allocated objects on each request 
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.
dc53a8c

rkh added a commit that referenced this pull request Oct 2, 2014

@rkh
Merge pull request #737 from schneems/schneems/less-objects 
Less allocated objects on each request
ab172af

@rkh rkh merged commit ab172af into rack:master Oct 2, 2014
1 check passed

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@rkh

This comment has been minimized.

Show comment
Hide comment
@rkh

rkh Oct 2, 2014

Member

Thank you! <3
Member

rkh commented Oct 2, 2014

Thank you! <3
@pedrocarrico

This comment has been minimized.

Show comment
Hide comment
@pedrocarrico

pedrocarrico Oct 2, 2014

A big 👍

pedrocarrico commented Oct 2, 2014

A big 👍
@tak1n

This comment has been minimized.

Show comment
Hide comment
@tak1n

tak1n Oct 2, 2014

Nice :)

tak1n commented Oct 2, 2014

Nice :)
lib/rack/body_proxy.rb
def respond_to?(*args)
return false if args.first.to_s =~ /^to_ary$/
super or @body.respond_to?(*args)
def respond_to?(method_name)

This comment has been minimized.

Show comment
Hide comment
@raggi

raggi Oct 2, 2014

Member

In 1.9, respond_to takes 2 arguments.
Copy URL
@raggi

raggi Oct 2, 2014

Member

In 1.9, respond_to takes 2 arguments.

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Oct 2, 2014

Contributor

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.
Copy URL
@schneems

schneems Oct 2, 2014

Contributor

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.

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Oct 2, 2014

Contributor

Fix is here: #739 sorry about that, should have known better. Thanks for the catch!
Copy URL
@schneems

schneems Oct 2, 2014

Contributor

Fix is here: #739 sorry about that, should have known better. Thanks for the catch!

This comment has been minimized.

Show comment
Hide comment
@raggi

raggi Oct 2, 2014

Member

Sorry, but false.object_id == 0, include_all in your example only takes a reference slot, not an RObject structure.
Copy URL
@raggi

raggi Oct 2, 2014

Member

Sorry, but false.object_id == 0, include_all in your example only takes a reference slot, not an RObject structure.

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Oct 3, 2014

Contributor

Updated in #742 also can get rid of a string allocation in method_missing
Copy URL
@schneems

schneems Oct 3, 2014

Contributor

Updated in #742 also can get rid of a string allocation in method_missing

schneems added a commit to schneems/rack that referenced this pull request Oct 2, 2014

@schneems
Fix: `respond_to?` takes 2 arguments 
The `respond_to?` method was optimized for less object creation, unfortunately I also mangled the method signature. This commit reverts the changes introduced in #737 to `BodyProxy#respond_to?` and adds tests.

See: https://github.com/rack/rack/pull/737/files#r18359323

cc/ @raggi
a9c0e35

@schneems schneems referenced this pull request Oct 2, 2014

Merged

Fix: `respond_to?` takes 2 arguments #739

soulim added a commit to soulim/rack that referenced this pull request Oct 2, 2014

@soulim
Follow-up for the pull request #737 
More commonly used strings converted into constants, and some strings are replaced with previously extracted constants.
f8b1ff2

@soulim soulim referenced this pull request Oct 2, 2014

Merged

Follow-up for the pull request #737 #741

@schneems schneems referenced this pull request Oct 2, 2014

Merged

docs for Reporter #6

lib/rack/runtime.rb
@@ -12,13 +12,14 @@ def initialize(app, name = nil)
@header_name << "-#{name}" if name
end
FORMAT_STRING = "%0.6f"

This comment has been minimized.

Show comment
Hide comment
@tjwallace

tjwallace Oct 3, 2014

Should be frozen?
Copy URL
@tjwallace

tjwallace Oct 3, 2014

Should be frozen?

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Oct 3, 2014

Contributor

Yep, good catch. Thanks! Updated in #742
Copy URL
@schneems

schneems Oct 3, 2014

Contributor

Yep, good catch. Thanks! Updated in #742
lib/rack/directory.rb
@@ -72,7 +72,7 @@ def check_forbidden
body = "Forbidden\n"
size = Rack::Utils.bytesize(body)
return [403, {"Content-Type" => "text/plain",

This comment has been minimized.

Show comment
Hide comment
@GetSetExecute

GetSetExecute Oct 3, 2014

Missed opportunity to replace "Content-Type" with CONTENT_TYPE. Also on line 132
Copy URL
@GetSetExecute

GetSetExecute Oct 3, 2014

Missed opportunity to replace "Content-Type" with CONTENT_TYPE. Also on line 132

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Oct 3, 2014

Contributor

RIght you are. I love open source, thanks for pointing this out! Updated in #742
Copy URL
@schneems

schneems Oct 3, 2014

Contributor

RIght you are. I love open source, thanks for pointing this out! Updated in #742

soulim added a commit to soulim/rack that referenced this pull request Oct 3, 2014

@soulim
Follow-up for the pull request #737 
More commonly used strings converted into constants, and some strings are replaced with previously extracted constants.
131d9fd

caius added a commit to caius/rack that referenced this pull request Oct 3, 2014

@caius
Backport rack#737 
This doesn't include the #respond_to? changes, because they were reverted in rack#739
a3212e1

caius added a commit to caius/rack that referenced this pull request Oct 3, 2014

@caius
Backport rack#737 
This doesn't include the #respond_to? changes, because they were reverted in rack#739
d635ad5

schneems added a commit to schneems/rack that referenced this pull request Oct 3, 2014

@schneems
Missed optimizations 
- freezing constant string to ensure it's not mutated
- use constant where available
- optimize `respond_to?` to take less memory. 

Discussed in #737 and #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
85a9929

@schneems schneems referenced this pull request Oct 3, 2014

Merged

Missed optimizations #742

schneems added a commit to schneems/rack that referenced this pull request Oct 3, 2014

@schneems
Missed optimizations 
- freezing constant string to ensure it's not mutated
- use constant where available
- optimize `respond_to?` to take less memory. 

Discussed in #737 and #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
9b922b5

schneems added a commit to schneems/rack that referenced this pull request Oct 3, 2014

@schneems
Missed optimizations 
- freezing constant string to ensure it's not mutated
- use constant where available
- optimize `respond_to?` to take less memory. 

Discussed in #737 and #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
751b8d5
@zaidakram

This comment has been minimized.

Show comment
Hide comment
@zaidakram

zaidakram Oct 5, 2014

Nice Stuff! 👍

zaidakram commented Oct 5, 2014

Nice Stuff! 👍
@stungeye

This comment has been minimized.

Show comment
Hide comment
@stungeye

stungeye Oct 5, 2014

👍

stungeye commented Oct 5, 2014

👍

schneems added a commit to schneems/rack that referenced this pull request Oct 6, 2014

@schneems
Missed optimizations 
- freezing constant string to ensure it's not mutated
- use constant where available
- optimize `respond_to?` to take less memory. 

Discussed in #737 and #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

schneems added a commit to schneems/rack that referenced this pull request Oct 6, 2014

@schneems
Missed optimizations 
- freezing constant string to ensure it's not mutated
- use constant where available
- optimize `respond_to?` to take less memory. 

Discussed in #737 and #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
5f808aa
@frodsan

This comment has been minimized.

Show comment
Hide comment
@frodsan

frodsan Jan 22, 2015

Contributor

@schneems The same change can be made in line 113 (env["PATH_INFO"])?
Contributor

frodsan commented on lib/rack/static.rb in dc53a8c Jan 22, 2015

@schneems The same change can be made in line 113 (env["PATH_INFO"])?

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Jan 22, 2015

Contributor

Probably. I never hit this in my benchmarks. Also more benchmarks here #742
Contributor

schneems replied Jan 22, 2015

Probably. I never hit this in my benchmarks. Also more benchmarks here #742

@u2 u2 referenced this pull request Mar 31, 2015

Closed

Less allocated objects on each request #973

@bquorning bquorning referenced this pull request Aug 7, 2015

Merged

Beyond Ludicrous Speed #21057

@BenMorganIO BenMorganIO referenced this pull request Aug 16, 2015

Closed

freeze some hotspots in ruby #1002

@marshall-lee marshall-lee referenced this pull request Jan 22, 2016

Merged

Enable frozen_string_literal feature. #1076

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