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

Improve eager load on Rails #7225

Merged
merged 6 commits into from Aug 21, 2012

Conversation

@josevalim
Copy link
Contributor

commented Aug 1, 2012

Today, eager loading an application is coupled with the config.cache_classes configuration. This means that every time we cache classes, we eager load the app. This is not necessarily true, for example in test environment an application could benefit of not eager loading the whole application when running a single test file, as the results reported here.

This proposal discuss some changes for Rails 4 to make booting more flexible and consistent.

config.eager_load

The first part of the proposal is to add a config.eager_load that says when to eager load the application. By default, we want these settings to be:

# development
config.cache_classes = false
config.eager_load = false

# test
config.cache_classes = true
config.eager_load = false # you may want to turn this to true if using spork

# production
config.cache_classes = true
config.eager_load = true

Notice that running a rake task always disables eager load (this is hardcoded in rails source today).

config.eager_load_namespaces

Rails also has a boolean configuration named config.preload_frameworks that preloads frameworks. I don't like this configuration because it is very tied to Rails and its frameworks. I propose instead config.eager_load_namespaces as a configuration generic enough not only to support Rails frameworks, but any gem or extension. We should be able to eager load any namespace by adding it to the list:

config.eager_load_namespaces << DataMapper

And now, whenever config.eager_load is true, we will invoke DataMapper.eager_load! to do the proper setup.

The idea of registering namespaces (and not lambdas) is that a user should be able to remove a namespace of the list if it is causing problems (or if they don't really need to eager load it).

Another nice thing about this approach is that all engines (and therefore any Rails application) already implements the eager_load! method, so they could be added by default to the config.eager_load_namespaces array, unifying both preload frameworks and eager load approaches.

NOTE: We are pending some hax to ensure production still work with webrick. At the worst scenario, bring the Rack::Lock option back.

@josevalim josevalim referenced this pull request Aug 1, 2012
@josevalim

View changes

activerecord/lib/active_record.rb Outdated
@@ -66,6 +66,12 @@ module ActiveRecord
autoload :Delegation
end

# TODO: Shrink this list down.

This comment has been minimized.

Copy link
@josevalim

josevalim Aug 1, 2012

Author Contributor

@jonleighton @rafaelfranca hey guys, could you please work on shrinking this list down? If you have questions, please ask me. But basically, everything that will be loaded at boot time if used (for example, Base and the modules it includes), needs to go. Feel free to do the change on master, I can rebase later.

This comment has been minimized.

Copy link
@spastorino

spastorino Aug 3, 2012

Member

I made this https://gist.github.com/e108d17ca5d9ae97135b most of them are used from Base.
What I'm not sure about is Migration, Migrator, Schema, SchemaDumper, SchemaMigration they are used in rake tasks so I don't want to load them when doing rails s, so I guess we shouldn't put that in eager load. Is that correct?

This comment has been minimized.

Copy link
@josevalim

josevalim Aug 3, 2012

Author Contributor

You are correct.

Sent from my iPhone

This comment has been minimized.

Copy link
@jonleighton

jonleighton Aug 3, 2012

Member

Thanks Santiago <3

This comment has been minimized.

Copy link
@spastorino
josevalim and others added 6 commits Aug 1, 2012
Allow users to choose when to eager_load the application or not.
Previously, the eager load behavior was mostly coupled to
config.cache_classes, however this was suboptimal since in
some environments a developer may want to cache classes but
not necessarily load them all on boot (for example, test env).

This pull request also promotes the use of config.eager_load
set to true by default in production. In the majority of the
cases, this is the behavior you want since it will copy most
of your app into memory on boot (which was also the previous
behavior).

Finally, this fix a long standing Rails bug where it was
impossible to access a model in a rake task when Rails was
set as thread safe.
Remove allow_concurrency as a flag
The flag was mainly used to add a Rack::Lock middleware to
the stack, but the only scenario the lock is desired is in
development.

If you are deploying on a not-threaded server, the Rack::Lock
does not provide any benefit since you don't have concurrent
accesses. On the other hand, if you are on a threaded server,
you don't want the lock, since it defeats the purpose of using
a threaded server.

If there is someone out there, running on a thread server
and does want a lock, it can be added to your environment
as easy as: `use Rack::Lock`
Make ActiveSupport::Autoload local
Previously, ActiveSupport::Autoload was global and reserved
for usage inside Rails. This pull request makes it local,
fixes its test (they were not being run because its file
was named wrongly) and make it part of Rails public API.
Get rid of config.preload_frameworks in favor of config.eager_load_na…
…mespaces

The new option allows any Ruby namespace to be registered and set
up for eager load. We are effectively exposing the structure existing
in Rails since v3.0 for all developers in order to make their applications
thread-safe and CoW friendly.
josevalim added a commit that referenced this pull request Aug 21, 2012
Merge pull request #7225 from rails/eager_load
Improve eager load on Rails

@josevalim josevalim merged commit 6bef146 into master Aug 21, 2012

1 check failed

default The Travis build failed
Details
@betelgeuse

This comment has been minimized.

Copy link

commented on railties/lib/rails/application.rb in e6747d8 Aug 22, 2012

Will ChangeLog entries be added in separate commits? I would guess that many people have made use of the global variable so we do want to give them proper notification that rails does not provide it any more to detect if you are running inside rake.

This comment has been minimized.

Copy link
Contributor

replied May 21, 2013

I actually just stumbled upon this.

@josevalim Could we properly deprecate this? I mean it's a global variable I always expected it to be public API because of that.

Compare c3ca7ac

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.