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

Memoize the `RouteSet#url_helpers` module #24554

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
@jeffrafter

jeffrafter commented Apr 14, 2016

Summary

This fixes a performance regression introduced between 4.1.x and 4.2.x.

Every call to a URL helper would create a new Module. Doing this repeatedly was not fast.

Fixes #23451

@rails-bot

This comment has been minimized.

rails-bot commented Apr 14, 2016

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @kaspth (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Apr 14, 2016

Thank you for the pull request. This method was not made to be called more than once in a request so it application responsibility to memoize it if you are calling more than once.

@jeffrafter

This comment has been minimized.

jeffrafter commented Apr 14, 2016

@rafaelfranca I'm still a bit confused. How do you memoize the instance? In this benchmark I grab a memoized instance and it is still slow. Is there a de facto way of doing this that I have missed? I modified my benchmark so that it does an include and it is still slow:

require 'benchmark'

class Thing
  include Buyer::Engine.routes.url_helpers

  def test
    campaign_url("sup")
  end
end
thing = Thing.new

Benchmark.bmbm do |x|
  x.report("url") {
    500.times do
      thing.test
    end
  }
end

And the results are still abysmal:

                                      user     system      total        real
url                               3.310000   0.050000   3.360000 (  3.573253)

I am not trying to be inflammatory but genuinely trying to understand how a common use can change so drastically without a deprecation warning leading to that change. Additionally, if this method is only intended to be called once then in all of the expected cases the memoization is a no-op... so it should do no harm to add it.

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Apr 14, 2016

I think that if you benchmark show it is slow when you have only one call to url_helper it is not the memoization that will fix this issue. Have you run your benchmark before and after your patch?

@jeffrafter

This comment has been minimized.

jeffrafter commented Apr 14, 2016

Yes, with the patch:

                                      user     system      total        real
url                               0.050000   0.010000   0.060000 (  0.061578)
@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Apr 14, 2016

could you share the benchmark application? I want to understand how a memoization in a method that is called only once could change that..

@jeffrafter

This comment has been minimized.

jeffrafter commented Apr 15, 2016

@rafaelfranca I couldn't publish the application I was working against so I made a new one: https://github.com/jeffrafter/sample-blorgh.

I used this benchmark:

require 'benchmark'

class Thing
  include Rails.application.routes.url_helpers

  def test
    testing_path
  end
end
thing = Thing.new

class Thing2
  include Blorgh::Engine.routes.url_helpers

  def test
    another_path
  end
end
thing2 = Thing2.new

Benchmark.bmbm do |x|
  x.report("url") {
    500.times do
      Rails.application.routes.url_helpers.testing_path
    end
  }
  x.report("included") {
    500.times do
      thing.test
    end
  }
  x.report("engine url") {
    500.times do
      Blorgh::Engine.routes.url_helpers.another_path
    end
  }
  x.report("engine included") {
    500.times do
      thing2.test
    end
  }
end

Rails 4.2 stable results

                      user     system      total        real
url               0.710000   0.010000   0.720000 (  0.728119)
included          0.000000   0.000000   0.000000 (  0.002965)
engine url        1.480000   0.020000   1.500000 (  1.580547)
engine included   0.800000   0.010000   0.810000 (  0.826432)

Rails 4.2 stable with patch results

                      user     system      total        real
url               0.000000   0.000000   0.000000 (  0.002621)
included          0.010000   0.000000   0.010000 (  0.004530)
engine url        0.040000   0.000000   0.040000 (  0.050396)
engine included   0.040000   0.000000   0.040000 (  0.051320)
@jeffrafter

This comment has been minimized.

jeffrafter commented Apr 15, 2016

I noticed when I was working through the examples that you're right... the default case with no engines is not degrading. But if you are calling the helpers without including them it slows dramatically. Additionally, anytime you use engines it degrades.

@jeffrafter

This comment has been minimized.

jeffrafter commented Apr 15, 2016

See also: #22320

@jeffrafter

This comment has been minimized.

jeffrafter commented Apr 15, 2016

The failure here is legit and a really good catch, I am looking into it.

jeffrafter
Memoize the `RouteSet#url_helpers` module
Within the url_helpers method a new `Module` is created
on each call. This change memoizes the module for performance.

Fixes #23451

@jeffrafter jeffrafter force-pushed the jeffrafter:master branch to d3028ea Apr 15, 2016

@jeffrafter

This comment has been minimized.

jeffrafter commented Apr 15, 2016

I pushed a change to this; the parameter supports_path was changing the behavior of the generated module so I am memoizing each possibility separately.

@jeffrafter

This comment has been minimized.

jeffrafter commented Apr 21, 2016

Is there anything I can do to help move this forward @rafaelfranca?

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Apr 21, 2016

We need to investigate why only engine routes have this problem and why this memoization helps in that case since it should only have one call to this method. Maybe the correct fix is there.

@bquorning

This comment has been minimized.

Contributor

bquorning commented Jun 6, 2016

I am seeing the same performance degradation, both using Rails 4.2.6 and Rails 5.0.0.rc1.

Every time you create a controller that inherits from ActionController::Base, AbstractController::Railties::RoutesHelpers.with will create a new routes module to include in that controller.

And rspec-rails will create a new module for every controller spec example: https://github.com/rspec/rspec-rails/blob/ac3276b7c8257ebac6db4bc023810b798237538c/lib/rspec/rails/example/view_example_group.rb#L180

@jimherz

This comment has been minimized.

Contributor

jimherz commented Jun 14, 2016

We are also seeing a lot of performance degradation due to this since we upgraded from Rails 4.2.4 to Rails 4.2.5.1. We decided to create a patch along the lines of this pull request to alleviate the situation.

I guess I'm having trouble understanding why the memoization is a problem here. My guess is that for the vast majority of Rails applications, route sets (and by extension their helpers) don't change once the application starts up. So, why then would Rails allow creation of multiple identical route helper modules for the same route set? If there is such a use case, I don't see it mentioned in the commit where the memoization was originally removed:

2bbcca0#diff-ccdef70ce0e95afc8816f47f89b56c60

@ajcumine

This comment has been minimized.

ajcumine commented Aug 22, 2016

@jeffrafter @rafaelfranca is there any movement on this issue? it's something we're currently monkey-patching ourselves but would be good to get it in rather than it getting lost/forgotten

@Lordnibbler

This comment has been minimized.

Lordnibbler commented Oct 14, 2016

@rafaelfranca any update on this? seeing this issue as well on rails 4.2.6

mariovisic added a commit to envato/guide that referenced this pull request Oct 17, 2016

Rails 4.2 performance fix
There exists [a bug introduced in rails 4.2](rails/rails#24554)
where calling the `url_helpers` of an engine will parse the entire set
of routes again and create a new object, as a result there is a huge
slowdown when looping, calling the `url_helpers` object again and again.

As the url helpers are included in the base controller, these methods
are available anyways, calling them this way doesn't parse all the
routes.

> Before: Completed 200 OK in 8337ms
> After: Completed 200 OK in 542ms
@Ragmaanir

This comment has been minimized.

Ragmaanir commented Mar 27, 2017

Hi,

we ran into the same issue and i want to contribute my findings:

Rails: 4.2.8, Ruby: 2.3.0

The url_helpers method in route_set.rb seems to be slow, just because it calls delegate:

[...]
class << self
  delegate :url_for, :optimize_routes_generation?, to: '@_routes' # takes 2 ms in our app

  attr_reader :_routes
  def url_options; {}; end
end
[...]

When i replace the call to delegate with explicit delegation via methods it is fast. Delegate might be slow because it invokes caller, but i did not profile that far.

Anyway, a regular rails app does not seem to be affected because the url_helpers is called only once. But when engines come into play this becomes an issue. This seems to be the cause:
define_generate_prefix

def define_generate_prefix(app, name)
  _route = @set.named_routes.get name
  _routes = @set
  app.routes.define_mounted_helper(name)
  app.routes.extend Module.new {
    def optimize_routes_generation?; false; end
    define_method :find_script_name do |options|
      if options.key? :script_name
        super(options)
      else
        prefix_options = options.slice(*_route.segment_keys)
        prefix_options[:relative_url_root] = ''.freeze
        # we must actually delete prefix segment keys to avoid passing them to next url_for
        _route.segment_keys.each { |k| options.delete(k) }
        _routes.url_helpers.send("#{name}_path", prefix_options)
      end
    end
  }
end

That method is invoked when calling mount. At the bottom the url_helpers method is invoked. So every time the find_script_name method is invoked, url_helpers is invoked.
find_script_name is invoked from url_for which in turn is called by UrlHelper#call.

@deivid-rodriguez

This comment has been minimized.

Contributor

deivid-rodriguez commented Jul 5, 2017

We need to investigate why only engine routes have this problem and why this memoization helps in that case since it should only have one call to this method. Maybe the correct fix is there.

@rafaelfranca I happened to study this part of the code for #29662, and @Ragmaanir's explanation makes sense to me. I didn't have the chance to notice the performance degradation myself, but maybe this PR is worth rebasing.

@Ragmaanir

This comment has been minimized.

Ragmaanir commented Jul 6, 2017

We definitely had performance problems due to this bug. My investigation at that time:

interesting: the url method seems to be really slow (at least for categories). when not outputting the url of categories the topics endpoint renders in 320ms, when enabling the url it renders in 650ms.

rendering 50 topics in the api without optimizations takes 3300ms. disabling url and one ability check: 500ms

@RaVbaker

This comment has been minimized.

RaVbaker commented Aug 2, 2017

Gems like jsonapi-resources when used from inside of an engine got a huge penalty due to this. Like pages are rendered way slower than they should. But I haven't yet found a nice solution. Although thanks @Ragmaanir for your detailed explanation. 🙇

@deivid-rodriguez

This comment has been minimized.

Contributor

deivid-rodriguez commented Aug 2, 2017

@RaVbaker You mean that this PR didn't make it fast?

@RaVbaker

This comment has been minimized.

RaVbaker commented Aug 2, 2017

@deivid-rodriguez Well not this one cause it might cause same issues as the previous version, not fresh enough if included on a module that already had the url_helpers included higher in the hierarchy. But my solution was more like I memoized the Engine.routes.url_helpers in my LinkBuilder so it doesn't lookup every time when url is generated but only once for each engine.

But it wasn't a full solution due to wha @Ragmaanir mentioned and for this I would recommend changing the define_generate_prefix method a bit with memoizing instead of _routes the _helpers and @set.url_helpers value in it and use that in line:

_helpers.send("#{name}_path", prefix_options)

This helped like a lot. On a page that normally generates around 5000ms, with only LinkBuilder memoization for url_helpers it get down into 3000ms but with that second change it ended at 1000ms. Which is 5 times faster. /cc: @rafal-brize

dmokreckiy added a commit to dmokreckiy/griddler-ses that referenced this pull request May 15, 2018

mtamhankar1 added a commit to idatainc/griddler-ses that referenced this pull request May 15, 2018

Fixed bad constant fetching and url_helpers performance bug (#2)
* Trying removing sinatra dependecies

* More dependecy changes

* Revert "More dependecy changes"

This reverts commit 43abadb.

* Returned middleware and sns_endpoint dependency

* Fixed bad use of Rails url_helpers rails/rails#24554

* Fixed bad constant reference

mtamhankar1 added a commit to idatainc/griddler-ses that referenced this pull request May 16, 2018

Enabled middleware for request headers patch (#3)
* Trying removing sinatra dependecies

* More dependecy changes

* Revert "More dependecy changes"

This reverts commit 43abadb.

* Returned middleware and sns_endpoint dependency

* Fixed bad use of Rails url_helpers rails/rails#24554

* Fixed bad constant reference

* Enabled middleware to patch up json headers

mtamhankar1 added a commit to idatainc/griddler-ses that referenced this pull request May 16, 2018

Reverted url_helperts patch (#4)
* Trying removing sinatra dependecies

* More dependecy changes

* Revert "More dependecy changes"

This reverts commit 43abadb.

* Returned middleware and sns_endpoint dependency

* Fixed bad use of Rails url_helpers rails/rails#24554

* Fixed bad constant reference

* Enabled middleware to patch up json headers

* Reverted url_helper patch because Rails.app is not present on class load

tenderlove added a commit that referenced this pull request Sep 24, 2018

Eagerly build the routing helper module after routes are committed
This commit eagerly builds the route helper module after the routes have
been drawn and finalized.  This allows us to cache the helper module but
not have to worry about people accessing the module while route
definition is "in-flight", and automatically deals with cache
invalidation as the module is regenerated anytime someone redraws the
routes.

The restriction this commit introduces is that the url helper module can
only be accessed *after* the routes are done being drawn.

Refs #24554 and #32892

tenderlove added a commit that referenced this pull request Sep 25, 2018

Eagerly build the routing helper module after routes are committed
This commit eagerly builds the route helper module after the routes have
been drawn and finalized.  This allows us to cache the helper module but
not have to worry about people accessing the module while route
definition is "in-flight", and automatically deals with cache
invalidation as the module is regenerated anytime someone redraws the
routes.

The restriction this commit introduces is that the url helper module can
only be accessed *after* the routes are done being drawn.

Refs #24554 and #32892
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment