Skip to content
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

Merged
merged 1 commit into from
Oct 2, 2014

Conversation

schneems
Copy link
Contributor

@schneems 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
Copy link

nthj commented Oct 2, 2014

👍 Yes please.

@bbwharris
Copy link

👍 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$/
Copy link
Contributor

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.

Copy link
Contributor Author

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
-----------------------------------

Copy link
Contributor Author

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.
@schneems schneems force-pushed the schneems/less-objects branch from f226179 to dc53a8c Compare October 2, 2014 12:42
rkh added a commit that referenced this pull request Oct 2, 2014
Less allocated objects on each request
@rkh rkh merged commit ab172af into rack:master Oct 2, 2014
@rkh
Copy link
Member

rkh commented Oct 2, 2014

Thank you! <3

@pedrocarrico
Copy link

A big 👍

@tak1n
Copy link

tak1n commented Oct 2, 2014

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)
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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!

Copy link
Member

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.

Copy link
Contributor Author

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

schneems added a commit to schneems/rack that referenced this pull request Oct 2, 2014
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"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be frozen?

Copy link
Contributor Author

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

caius added a commit to caius/rack that referenced this pull request Oct 3, 2014
This doesn't include the #respond_to? changes, because they were reverted in rack#739
caius added a commit to caius/rack that referenced this pull request Oct 3, 2014
This doesn't include the #respond_to? changes, because they were reverted in rack#739
schneems added a commit to schneems/rack that referenced this pull request Oct 3, 2014
@schneems schneems mentioned this pull request Oct 3, 2014
schneems added a commit to schneems/rack that referenced this pull request Oct 3, 2014
schneems added a commit to schneems/rack that referenced this pull request Oct 3, 2014
@zaidakram
Copy link

Nice Stuff! 👍

@stungeye
Copy link

stungeye commented Oct 5, 2014

👍

schneems added a commit to schneems/rack that referenced this pull request Oct 6, 2014
schneems added a commit to schneems/rack that referenced this pull request Oct 6, 2014
@schneems schneems changed the title Less allocated objects on each request Fewer allocated objects on each request Feb 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.