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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Eager load controller actions to reduce response time of the first request #29559

Merged
merged 1 commit into from Aug 11, 2017
Merged

Eager load controller actions to reduce response time of the first request #29559

merged 1 commit into from Aug 11, 2017

Conversation

@kirs
Copy link
Member

@kirs kirs commented Jun 24, 2017

On the first request, ActionController::Base#action_methods computes
and memoizes
the list of available actions. With this PR we move this expensive operation into eager load step to reduce response time of the first request served in production.

This also reduces the memory footprint when running on forking server like Unicorn.

all credit goes to @casperisfine. I'm just pushing this patch from Shopify into upstream 馃槉

@rafaelfranca @Edouard-chin

@rails-bot
Copy link

@rails-bot rails-bot commented Jun 24, 2017

r? @matthewd

(@rails-bot has picked a reviewer for you, use r? to override)

railties/test/railties/engine_test.rb Outdated

boot_rails

assert_not_called(BukkitController, :internal_methods, "#action_methods is already memoized") do

This comment has been minimized.

@kirs

kirs Jun 24, 2017
Author Member

Another way to test it would be checking the memoized instance variable:

assert BukkitController.instance_variable_get(:@action_methods)

But rather I prefered to check that the depending methods are not invoked.

actionpack/lib/action_controller.rb Outdated
@@ -54,6 +54,12 @@ module ActionController
autoload :TemplateAssertions, "action_controller/test_case"
end

ActiveSupport.on_load(:after_initialize) do

This comment has been minimized.

@kirs

kirs Jun 24, 2017
Author Member

It would be way more clean to put it into ActionController#eager_load!, however when that one is invoked the app controllers itself are not loaded yet.

I thought that maybe calling ActiveSupport.on_load inline from the root component class is shady but then I found other components doing it too: https://github.com/rails/rails/blob/master/actionpack/lib/action_dispatch.rb#L106

This comment has been minimized.

@rafaelfranca

rafaelfranca Jun 26, 2017
Member

We should move this to the railtie

actionpack/lib/action_controller.rb Outdated
@@ -54,6 +54,12 @@ module ActionController
autoload :TemplateAssertions, "action_controller/test_case"
end

ActiveSupport.on_load(:after_initialize) do
if config.eager_load
ActionController::Base.descendants.each(&:action_methods)

This comment has been minimized.

@kirs

kirs Jun 24, 2017
Author Member

Instead of iterating over descendants here we could add ActiveSupport.on_load(:after_initialize) callback every time new controller is inherited. However that would result to a ton on AS callbacks in case of an large app with hundreds of controllers.

Let me know if you think we'd prefer creating a callback per controller.

actionpack/lib/action_controller.rb Outdated
@@ -54,6 +54,12 @@ module ActionController
autoload :TemplateAssertions, "action_controller/test_case"
end

ActiveSupport.on_load(:after_initialize) do

This comment has been minimized.

@rafaelfranca

rafaelfranca Jun 26, 2017
Member

We should move this to the railtie

actionpack/lib/action_controller.rb Outdated
@@ -54,6 +54,12 @@ module ActionController
autoload :TemplateAssertions, "action_controller/test_case"
end

ActiveSupport.on_load(:after_initialize) do
if config.eager_load
ActionController::Base.descendants.each(&:action_methods)

This comment has been minimized.

@rafaelfranca

rafaelfranca Jun 26, 2017
Member

How about subclasses on ActionController::Api, ActionController::Metal and ActionMailer::Base? I think we should use ActionController::Metal here and do the same with ActionMailer::Base in its railtie.

This comment has been minimized.

@matthewd

matthewd Aug 2, 2017
Member

Why not go straight to AbstractController::Base?

This comment has been minimized.

@kirs

kirs Aug 5, 2017
Author Member

@matthewd AbstractController::Base.descendants would also include ActionMailer, and I thought we'd want to isolate eager loading controller actions and mailer actions.

With the current state of the PR, ActionMailer actions would be eager loaded from a separate ActionMailer-specific callback.

Do you think it makes sense to use only one callback to eager load actions from both ActionController and ActionMailer?

@kirs
Copy link
Member Author

@kirs kirs commented Jul 1, 2017

@rafaelfranca I addressed your feedback. Thanks for review!

@kirs
Copy link
Member Author

@kirs kirs commented Jul 9, 2017

railties/test/railties/engine_test.rb Outdated
@@ -252,6 +254,64 @@ def index
assert_equal "Hello bukkits\n", response[2].body
end

test "does not eager load controller actions in development" do

This comment has been minimized.

@rafaelfranca

rafaelfranca Jul 24, 2017
Member

Those tests seems to be in the wrong place. Should not this be in the application/configuration_test? While engines defines initializers they don't boot the application so eager load tests should not be here.

https://github.com/rails/rails/blob/master/railties/test/application/configuration_test.rb

On the first request, ActionController::Base#action_methods computes
and memoized the list of available actions [1]. With this PR we move
this expensive operation into eager load step to reduce response time
of the first request served in production.

This also reduces the memory footprint when running on forking server
like Unicorn.

[1] https://github.com/rails/rails/blob/a3813dce9a0c950a4af7909111fa730a2622b1db/actionpack/lib/abstract_controller/base.rb#L66-L77
@kirs
Copy link
Member Author

@kirs kirs commented Jul 29, 2017

@rafaelfranca thanks for review! I addressed the feedback. CI is green.

@rafaelfranca rafaelfranca merged commit b9e0b4f into rails:master Aug 11, 2017
2 checks passed
2 checks passed
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@matthewd
Copy link
Member

@matthewd matthewd commented Nov 18, 2017

@kirs @rafaelfranca this should be using eager_load_namespaces instead of after_initialize

@kirs
Copy link
Member Author

@kirs kirs commented Nov 19, 2017

@matthewd I tried it and the problem is that when eager_load_namespaces are loaded, the app code itself is not loaded yet. As a result ActionMailer::Base.descendants would always be [] when called within eager_load_namespaces.

In contrast, when we do it in after_initialize the app code is already loaded and ActionMailer::Base.descendants would include all classes that we expect.

Do you think it's a bigger problem that we should fix?

@matthewd
Copy link
Member

@matthewd matthewd commented Nov 19, 2017

Ah, hrmm. I was just looking at after_initialize callbacks and these stood out as seeming like they were in the wrong place. My direct concern is for other after_initialize callbacks that might rely on the fact "everything of note" has already occurred (e.g. AR).

At a more fundamental level, if we can't even implement our own eager loading using the callback designed for it... 馃槙

Maybe we should be splitting eager loading into two phases, constant loading vs "extra" loading/preparation?

Or maybe this is fine as it is. It just feels weird to me -- these components shouldn't really know about config.eager_load at all.

@kirs kirs deleted the kirs:eager-load-controller-actions branch Nov 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants