Ensure application gets reloaded only once by jscheid · Pull Request #5740 · activeadmin/activeadmin

jscheid · 2019-04-13T10:41:52Z

I've recently used ruby-prof to look at reload performance, like so:

require 'ruby-prof'

result = RubyProf.profile do
  Rails.application.reloader.reload!
end
printer = RubyProf::MultiPrinter.new(result)
printer.print(path: "/path/to/output", profile: "reload-profile")

Doing so revealed that ActiveAdmin.application.load! is called twice. Since that method accounts for a substantial portion of the time it takes to reload our app, this seems quite wasteful.

The reason is that the prepare callback is invoked twice, from here:

https://github.com/rails/rails/blob/v5.2.3/activesupport/lib/active_support/reloader.rb#L46

and from here:

https://github.com/rails/rails/blob/v5.2.3/activesupport/lib/active_support/reloader.rb#L59

For the "load" callback alone this wouldn't be a problem because it only calls load! if the application is not already loaded. However, the "unload" callback also uses the prepare callback, which results in the following order of callbacks:

unload
load
unload
load

That defeats the loaded? guard because by the time the "load" callback is invoked a second time, the application has been unloaded again.

This PR attempts to solve the problem by using the class_unload event instead of the prepare event for the "unload" callback. That event is only invoked once per reload.

It also removes the compatibility layer in reloader.rb. As far as I can tell, it is no longer needed now that support for Rails 4.x has been dropped.

It also simplifies the logic for registering the "unload" callback because the same callback works regardless of the setting for reload_classes_only_on_change.

Closes #5739

deivid-rodriguez · 2019-04-13T11:56:20Z

I love this ❤️.

One thought and two nitpicks.

Maybe we should land the #5732 PR first, in case this conflicts with Zeitwerk. In particular, I think the spec you have added will need to run with the CLASS_RELOADING environment variable set (which configures Rails cache_classes on our test app), because Zeitwerk explicitly forbids reloading unless that setting is enabled.

As nitpicks, I'd love if you could split the PR into two separate commits, one that removes the unnecessary compatibility layer with the Rails reloader, and another one that fixes the double loading issue.

The other one is that the "integration_spec.rb" file we could maybe be renamed to "rails_integration_spec.rb"

jscheid · 2019-04-13T12:02:37Z

Sure, no problem. I’ll get to it tomorrow or Monday.

jscheid · 2019-04-13T20:51:56Z

@deivid-rodriguez all done. I've also merged your zeitgeist branch into my local to see how the two work together. The test seems to pass fine without CLASS_RELOADING set. Perhaps because no code has changed before the reload is triggered?

(And yes I've run the specs with export BUNDLE_GEMFILE=gemfiles/rails_60.gemfile 😺)

* 5739-reload-once: Ensure application gets reloaded only once Remove reloader compatibility layer

deivid-rodriguez · 2019-04-25T10:31:19Z

This looks good to me! Thanks @jscheid!

Just in case, can you rebase this since there's been a few changes in zeitwerk and rails since last time?

deivid-rodriguez · 2019-04-25T10:36:02Z

Oh, can you also add a changelog entry for the reloading fix?

jscheid · 2019-04-25T11:09:37Z

@deivid-rodriguez rebased... could you add the change log please? I don't know what version it will be released in.

deivid-rodriguez · 2019-04-25T11:10:57Z

You don't need to know it, put it under the "Unreleased" section. Anyways, I can do it, no problem 😃

jscheid · 2019-04-25T11:23:17Z

Ah, now it did break. Good call. Curious, because it definitely didn't the other day (with Zeitwerk merged in). I'll have another look at this tomorrow 😕

deivid-rodriguez · 2019-04-25T11:25:05Z

Great, thanks!

deivid-rodriguez · 2019-04-25T11:25:29Z

I plan to release this with 2.1 by the way, together with "official" Rails 6 support.

This is no longer needed now that support for Rails 4.x has been dropped.

jscheid · 2019-04-25T13:25:02Z

@deivid-rodriguez any idea what the codeclimate failure is about? I'm inclined to think it's a bug... I had pushed the same change before with the only difference in the change log, and it didn't complain.

jscheid · 2019-04-25T13:26:57Z

~~Oh, maybe it didn't run because of the conflict... anyway, do you know off the top of your head how this can be resolved?~~ Never mind, it wasn't running the new spec type in CI.

deivid-rodriguez · 2019-04-25T13:29:01Z

Since you're adding a new "suite", you need to make sure coverage reports are properly merged together. You should create a new spec/support/simplecov_reload_env.rb file doing something similar to spec/support/simplecov_regular_env.rb and spec/support/simplecov_changes_env.rb.

jscheid · 2019-04-25T13:30:07Z

@deivid-rodriguez there are already cucumber specs for this type. Were they not considered for code coverage before?

deivid-rodriguez · 2019-04-25T13:32:07Z

Cucumber reloading specs are also part of a separate suite, which is properly configured for coverage report merging. Basically every set of specs/features that run as a separate process needs custom code so that their coverage reports are properly merged.

jscheid · 2019-04-25T13:33:09Z

Ah, I think we're good: https://github.com/activeadmin/activeadmin/blob/master/features/support/simplecov_reload_env.rb

I think problem was just that the new type didn't run in CI. 🤞

Closes activeadmin#5739

jscheid · 2019-04-25T14:06:10Z

You were right, that file was missing.

What a drag... you're lucky you got your PR in before mine 😉

Should be all good now?

deivid-rodriguez

❤️

javierjulio

Looks great, thank you! 👏🏻

jscheid · 2019-04-26T02:12:20Z

You're welcome, thanks for merging.

jscheid force-pushed the 5739-reload-once branch from f10d5e5 to 86fe8eb Compare April 13, 2019 20:27

varyonic added a commit to activeadmin-rails/activeadmin-rails that referenced this pull request Apr 19, 2019

Merge branch '5739-reload-once' (activeadmin#5740)

57cb9ee

* 5739-reload-once: Ensure application gets reloaded only once Remove reloader compatibility layer

deivid-rodriguez previously approved these changes Apr 25, 2019

View reviewed changes

jscheid dismissed deivid-rodriguez’s stale review via cac0589 April 25, 2019 11:07

jscheid force-pushed the 5739-reload-once branch from 86fe8eb to cac0589 Compare April 25, 2019 11:07

jscheid force-pushed the 5739-reload-once branch from cac0589 to b2e5f7c Compare April 25, 2019 12:16

Remove reloader compatibility layer

77b4d5f

This is no longer needed now that support for Rails 4.x has been dropped.

jscheid force-pushed the 5739-reload-once branch from b2e5f7c to dbde7d8 Compare April 25, 2019 12:58

jscheid force-pushed the 5739-reload-once branch from dbde7d8 to 66e40c4 Compare April 25, 2019 13:28

Ensure application gets reloaded only once

21b6138

Closes activeadmin#5739

jscheid force-pushed the 5739-reload-once branch from 66e40c4 to 21b6138 Compare April 25, 2019 13:43

javierjulio requested a review from deivid-rodriguez April 25, 2019 14:04

deivid-rodriguez approved these changes Apr 25, 2019

View reviewed changes

javierjulio approved these changes Apr 25, 2019

View reviewed changes

javierjulio merged commit 21e0f82 into activeadmin:master Apr 25, 2019

jscheid deleted the 5739-reload-once branch April 26, 2019 02:12

deivid-rodriguez mentioned this pull request Jun 20, 2019

reloading much slower after 2.1 upgrade in development mode for a larger code base and about 25 active admin entities #5787

Closed

deivid-rodriguez mentioned this pull request Jul 10, 2019

Revert "Ensure application gets reloaded only once" #5801

Merged

Uh oh!

Conversation

jscheid commented Apr 13, 2019

Uh oh!

deivid-rodriguez commented Apr 13, 2019

Uh oh!

jscheid commented Apr 13, 2019

Uh oh!

jscheid commented Apr 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deivid-rodriguez commented Apr 25, 2019

Uh oh!

deivid-rodriguez commented Apr 25, 2019

Uh oh!

jscheid commented Apr 25, 2019

Uh oh!

deivid-rodriguez commented Apr 25, 2019

Uh oh!

jscheid commented Apr 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deivid-rodriguez commented Apr 25, 2019

Uh oh!

deivid-rodriguez commented Apr 25, 2019

Uh oh!

jscheid commented Apr 25, 2019

Uh oh!

jscheid commented Apr 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deivid-rodriguez commented Apr 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jscheid commented Apr 25, 2019

Uh oh!

deivid-rodriguez commented Apr 25, 2019

Uh oh!

jscheid commented Apr 25, 2019

Uh oh!

jscheid commented Apr 25, 2019

Uh oh!

deivid-rodriguez left a comment

Choose a reason for hiding this comment

Uh oh!

javierjulio left a comment

Choose a reason for hiding this comment

Uh oh!

jscheid commented Apr 26, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jscheid commented Apr 13, 2019 •

edited

Loading

jscheid commented Apr 25, 2019 •

edited

Loading

jscheid commented Apr 25, 2019 •

edited

Loading

deivid-rodriguez commented Apr 25, 2019 •

edited

Loading