Skip to content
This repository

Performance regression in route compilation #2346

Closed
bradediger opened this Issue · 11 comments

3 participants

Brad Ediger Jon Leighton Dmitriy Kiriyenko
Brad Ediger

9be7911 introduced a significant performance regression in application startup when a Rails application has many routes (on the order of hundreds). Each route defines its hash accessor and url helper methods, but first calls remove_possible_method(selector) on the helper module, presumably to silence possible warnings about method redefinition. Here is the code I'm talking about:

https://github.com/rails/rails/blob/master/actionpack/lib/action_dispatch/routing/route_set.rb#L165

However, remove_possible_method is slow in the typical case where the method doesn't exist, because it calls remove_method and rescues the resulting NameError. This can add seconds to application startup when there are a few dozen to a few hundred routes.

Since the semantics don't appear to be changed by 9be7911, I'd kindly request that that commit be reverted.

Here is my test methodology:


Setup:

  1. Pull down Rails master (tested @ 455e9e7)
  2. rvm 1.9.2-p290
  3. rails-edge/bin/rails new testapp --dev && cd testapp

Profile startup of the baseline (empty) application:

  1. vim config/routes.rb:
Testapp::Application.routes.draw do
end
  1. time script/rails runner 1

script/rails runner 1 4.44s user 0.95s system 100% cpu 5.384 total

Add many routes to routes.rb and profile again:

  1. vim config/routes.rb:
Testapp::Application.routes.draw do
  400.times do |i|
    resources "route#{i}"
  end
end
  1. time script/rails runner 1

script/rails runner 1 16.21s user 1.18s system 94% cpu 18.446 total

At this point, profiling the startup (ruby-prof -p graph_html -f startup_profile.html -m 1 script/rails runner 1) will show that Module#remove_method is taking a significant amount of time (about 9% in this example, but it was about 20% of startup time in the original application where I discovered this issue). Reverting that single commit reduces the startup time significantly:

Revert 9be7911 and run again:

  1. cd ../rails-edge
  2. git revert 9be7911e (there are trivial conflicts in the header... merge them)
  3. cd ../testapp
  4. time script/rails runner 1

script/rails runner 1 11.46s user 1.06s system 100% cpu 12.513 total

Jon Leighton
Owner

Thanks for reporting. I have assigned this as a blocker for 3.1. Ideally we should fix remove_possible_method to not use exceptions for flow control :/

Brad Ediger

Thanks for the quick response. I agree that remove_possible_method is the right place for this, after seeing everywhere it's currently being used across Rails.

Unfortunately, the "obvious" fix, which works for the specific case of route helper methods, won't work in general:

def remove_possible_method(method)
  remove_method(method) if method_defined?(method)
rescue NameError # ignore
end

because method_defined? ignores private methods. (The NameError rescue is still necessary in case the method in question came from a superclass or included module -- in which case method_defined? returns true but the method can't be removed from self. However, that rescue should be hit much less often.)

I'm not quite sure how to approach this one in the general case. I'll think on it a bit, but if you have any ideas I'm open to them. Thanks again!

Brad Ediger

I meant to provide an example of where the public/private distinction actually matters to remove_possible_method. Here's a concrete example:

https://github.com/rails/rails/blob/master/actionpack/lib/abstract_controller/layouts.rb#L283

Since _layout is marked private, the naïve reimplementation of remove_possible_method above won't see that it is defined and thus won't remove it before redefining it. This triggers a redefinition warning.

Dmitriy Kiriyenko

In general I'd say when we don't know if a method exists but wish to redefine it is a kind'a smell. There's a conventional way of method redefinition that does not give a warning: inheritance and/or mixins. We have lots of design patterns to do the things the right way, not the monkey patching.

My second point is that warning is like pain - it's uncomfortable but it's useful as long as it's meaningful. May I ask what possible earlier existing methods are redefined by creating a hash accessor and url helper methods? If it's methods, created by previous routes, I guess, the warning should be in place. It's a correct behavior. Same as with delegation: fcbde45 recent change.

Jon Leighton
Owner

We can use private_method_defined? to detect the private methods:

def remove_possible_method(method)
  remove_method(method) if method_defined?(method) || private_method_defined?(method)
rescue NameError # ignore
end

However, I completely agree with @dmitriy-kiriyenko's points, it would be great to cut back our use of method removal, but I think the above is a decent quick fix in the mean time.

Brad Ediger bradediger referenced this issue from a commit
Commit has since been removed from the repository and is no longer available.
Brad Ediger

I agree with all of the points made above about warnings being a code smell. However, there's a lot of surface to cover in pulling this out, and I'm not sure it will be easy to cover them all.

@jonleighton: thanks for the hint on private_method_defined?. Somehow I skipped right over that. I've included the stopgap fix on my fork, bradediger/rails@issue-2346. I show it yielding about a 20% speedup in application startup on the 400-route example in my OP.

Thanks!

Jon Leighton
Owner

@bradediger please could you put this in a pull request? (one for master, and one for 3-1-stable) Thanks!

Brad Ediger

#2371 created for master. It looks like Github won't let me create two pull requests with different destination branches; I'm not too familiar with the new pull request system. I can definitely try to create a new pull request for 3-1-stable if #2371 is merged first.

Jon Leighton
Owner

For 3-1-stable, you'll need to create a new branch from origin/3-1-stable, then cherry pick your commit, then push the branch to your repo and submit a pull request from that. Cheers

Dmitriy Kiriyenko

Wow, private_method_defined even exists in 1.8.7. Cool. Somehow I missed it.

Brad Ediger bradediger referenced this issue from a commit in bradediger/rails
Brad Ediger bradediger remove_possible_method: test if method exists
This speeds up remove_possible_method substantially since it doesn't
have to rescue a NameError in the common case.

Closes #2346.
f2657b0
Brad Ediger

@jonleighton: /facepalm. Of course. Sorry, it's an early Sunday morning here in Chicago ;-)

#2372 is the 3-1-stable port.

Jon Leighton jonleighton closed this issue from a commit
Brad Ediger bradediger remove_possible_method: test if method exists
This speeds up remove_possible_method substantially since it doesn't
have to rescue a NameError in the common case.

Closes #2346.
7f88539
Arun Agrawal arunagw referenced this issue from a commit in arunagw/rails
Brad Ediger bradediger remove_possible_method: test if method exists
This speeds up remove_possible_method substantially since it doesn't
have to rescue a NameError in the common case.

Closes #2346.
e1b5464
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.