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

Defer route drawing to the first request, or when url_helpers called #51614

Merged
merged 1 commit into from May 12, 2024

Conversation

gmcgibbon
Copy link
Member

@gmcgibbon gmcgibbon commented Apr 19, 2024

Motivation / Background

This Pull Request has been created because apps with lots of routes take a long time to boot. A developer could boot an app for reasons that don't involve routes at all (like running unit tests, migrations, rake tasks, etc.) so I think this should be deferred in dev and test.

Detail

This Pull Request changes engine and app route sets to a Rails::Engine::RouteSet, which knows about the current Rails application. The default middleware stack has also changed to include a Rails::Rack::LoadRoutes middleware that loads routes if needed. This PR loads routes under the following circumstances:

In dev/test:

  • The first request via middleware
  • When application or engine url_helpers.some_path is called via method_missing?
  • When application or engine url_helpers.respond_to?(:some_path) is called via respond_to_missing?

In production:

  • In the finisher eagerly, which is the previous behaviour

If, for some reason, a developer wishes to revert to the previous behaviour, they could add an initializer with Rails.application.reload_routes!. However, if we wanted to be more safe, we could also hide this behaviour behind a configuration variable.

Additional information

This is loosely inspired by https://github.com/amatsuda/routes_lazy_routes

I'm a bit concerned this is too much of a hack, but I think the speed benefit might be worth it. In an app with 2000 routes:

Before:

% time bin/rails runner ""
bin/rails runner ""  0.66s user 0.33s system 95% cpu 1.036 total

After:

% time bin/rails runner ""
bin/rails runner ""  0.47s user 0.32s system 98% cpu 0.791 total

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Unrelated changes should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@gmcgibbon gmcgibbon force-pushed the defer_route_drawing branch 3 times, most recently from b0b0a0e to f12328c Compare April 20, 2024 07:44
@rails-bot rails-bot bot added the actionview label May 3, 2024
@gmcgibbon gmcgibbon force-pushed the defer_route_drawing branch 8 times, most recently from df3e1ba to 94871b1 Compare May 3, 2024 19:24
@gmcgibbon
Copy link
Member Author

Should be green now. I'm using method_missing and middleware to load routes instead of doing it eagerly, and it seems to work pretty well, but I'd like some feedback before I merge.

@gmcgibbon
Copy link
Member Author

cc @casperisfine since you had opinions about the gem that did this. Does this seem like an acceptable approach to you?

@gmcgibbon
Copy link
Member Author

I think this might break routing test helpers in Rails where applications draw test routes. The middleware may break that, so I'll check before merging.

Executes the first routes reload in middleware, or when the route set
url_helpers is called. Previously, this was executed unconditionally on
boot, which can slow down boot time unnecessarily for larger apps with
lots of routes.
@casperisfine
Copy link
Contributor

Does this seem like an acceptable approach to you?

I only had a quick look, but yes, seem good to me.

@Fryguy
Copy link
Contributor

Fryguy commented May 10, 2024

I opened an issue along time ago about how the removal of dynamic actions caused our number of routes to explode, and used a ton more memory: #27231. Since then we had switch to routes_lazy_routes, but I would prefer to remove that gem and have it baked in, so I'm looking forward to this PR.

@gmcgibbon gmcgibbon merged commit e97db3b into rails:main May 12, 2024
4 checks passed
@gmcgibbon gmcgibbon deleted the defer_route_drawing branch May 12, 2024 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants