-
Notifications
You must be signed in to change notification settings - Fork 21.7k
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
Add bootsnap to default Gemfile #29313
Conversation
Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @eileencodes (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. This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review. Please see the contribution instructions for more information. |
load_path_cache: true, | ||
autoload_paths_cache: true, | ||
compile_cache_iseq: true, | ||
compile_cache_yaml: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is way too much / too specific configuration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should follow bundlers lead:
require "bundler/setup" # Set up gems listed in the Gemfile
require "bootsnap/setup" # Caches and compiles gems for faster boot.
Which then just includes the above conventionalized config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could also be require "bootsnap/railtie"
or Rails could just set up bootsnap automatically after executing boot.rb
if the gem is present, e.g.:
# Somewhere in the Rails bowels:
begin
require "bootsnap"
Bootsnap.setup(…)
rescue LoadError
end
@@ -1,3 +1,17 @@ | |||
ENV['BUNDLE_GEMFILE'] ||= File.expand_path('../Gemfile', __dir__) | |||
|
|||
require 'bundler/setup' # Set up gems listed in the Gemfile. | |||
|
|||
if RUBY_ENGINE == 'ruby' && RUBY_PLATFORM =~ /darwin|linux|bsd/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should always require bootsnap and let it decide whether it supports the current environment.
@@ -25,6 +25,10 @@ git_source(:github) { |repo| "https://github.com/#{repo}.git" } | |||
|
|||
<%- end -%> | |||
<% if RUBY_ENGINE == 'ruby' -%> | |||
|
|||
# Reduces boot times through caching; configured in config/boot.rb | |||
gem 'bootsnap', '~> 1.0.0', platforms: [:mri] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we loosen this version constraint a bit?
Can bootsnap be installed on all platforms, even if it won't Do Cool Stuff?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bootsnap only supports Ruby 2.3.0 and higher, but Rails still supports Ruby 2.2.
https://github.com/Shopify/bootsnap/blob/master/bootsnap.gemspec#L26
Should we check the Ruby version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@burke could bootsnap allow installation on 2.2, and just operate in a degraded mode? AIUI path caching would still work -- it's the iseq cache that is strictly 2.3+, right?
This is not necessary thing for most Rails apps. Things must be kept as simple as possible. In future this change may bring some bugs. If people need this it is better to create separate official gem |
Ok -- I don't have any time this week to revise this, but here's what I'm going to do:
The method used here will still be available, but bootsnap/setup will wrap it. |
@yivo we are not merging anything. It is not even being added as a dependency of a framework. It is being added to the generated Gemfile and if people don't want they can just remove. Rails give people sane defaults so they don't need to spend time thinking if they want to use something or not. @burke seems like a good plan 👍 |
cf1b495
to
71719bb
Compare
I've updated this PR to use bootsnap-1.1.0.pre. I'll update again to 1.1.0 after some more testing/feedback. Changes:
See also Thoughts? |
@@ -19,6 +19,9 @@ git_source(:github) { |repo| "https://github.com/#{repo}.git" } | |||
# Use Capistrano for deployment | |||
# gem 'capistrano-rails', group: :development | |||
|
|||
# Reduces boot times through caching; required in config/boot.rb | |||
gem 'bootsnap', '~> 1.1.0.pre', require: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need a conservative ~>
dep? How about >= 1.1.0.pre
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, updated.
7fa15d0
to
78a4d76
Compare
|
Yeah -- it's unfortunately hard to detect if coverage is currently enabled. Ruby, perplexingly, doesn't provide an API for this. |
Not sure what can I be doing wrong but adding
I've tested this a dozen times getting similar numbers every time. |
Are you running it in a VM? Is the application source in a shared folder? If so, set bootsnap's cache directory outside of your app, or symlink |
No and no. I tried removing the tmp/cache folder anyway and creating a symlink under /tmp but I don't have luck yet. During the first run (when cache isn't created yet) the time is very similar to the normal boot time of the app:
During subsequent boots the time goes up to 2x the normal time:
|
Very strange, so it sounds like What do you get back from:
|
It might make sense to move this to an issue on bootsnap. |
@burke sure!! Shopify/bootsnap#68 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one more thing… plus squash your commits and add a changelog entry. Then I think we're good to go! 🤘
@@ -56,6 +56,9 @@ gem "libxml-ruby", platforms: :ruby | |||
# Action View. For testing Erubis handler deprecation. | |||
gem "erubis", "~> 2.7.0", require: false | |||
|
|||
# for railties app_generator_test | |||
gem "bootsnap", ">= 1.1.0", require: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like us needing to bundle Bootsnap in the main gemfile for our Railties tests to work. Can't we find out what Gemfile template / gems the Railties generated Rails app uses? @rafaelfranca are you familiar with gems and railties tests?
Or maybe we should just use require "bootsnap/setup" rescue LoadError
in config/boot.rb
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The application inside the railties test suite uses the Rails main Gemfile. I don't see a problem on adding bootsnap in the main gemfile, we already do to gems like jbuilder, erubis, etc.
Bootsnap precomputes load path resolution and caches ruby ISeq and YAML parsing/compilation, reducing application boot time by approximately 50% on supported configurations.
Rebased, squashed, and added changelog entry. I'm not against searching for an alternative solution if the addition to the root Gemfile is unacceptable on balance, but I'm strongly against rescuing |
I was just going to merge this, but @rafaelfranca's too fast for me! 😄 |
🎉 🎉 🎉 |
$ bin/rails test /Users/prathamesh/Projects/sources/codetriage/config/boot.rb:4:in `require': cannot load such file -- bootsnap/setup (LoadError) from /Users/prathamesh/Projects/sources/codetriage/config/boot.rb:4:in `<top (required)>' from bin/rails:3:in `require_relative' from bin/rails:3:in `<main>' - Rails uses bootstanp greater than 1.1.0 ~ rails/rails#29313
$ bin/rails test /Users/prathamesh/Projects/sources/codetriage/config/boot.rb:4:in `require': cannot load such file -- bootsnap/setup (LoadError) from /Users/prathamesh/Projects/sources/codetriage/config/boot.rb:4:in `<top (required)>' from bin/rails:3:in `require_relative' from bin/rails:3:in `<main>' - Rails uses bootstanp greater than 1.1.0 ~ rails/rails#29313
### Description Rails 5.2 ships with a new default gem called `bootsnap`, which [speeds up Rails boot time by 50%](rails/rails#29313). I've added it here to see if it'll work for Caseflow. Faster boot times mean a shorter development loop, less CI contention, and faster boot/restart during deploys. [How it works](https://github.com/Shopify/bootsnap#how-does-this-work) tl;dr avoiding slow scanning of $LOAD_PATH and caching of yaml files (both are slow parts of Rails boot) Results for caseflow, locally testing a couple results of `time bundle exec rails s` until the 'Ctrl-C' prompt appears: before: real 0m8.811s real 0m8.325s after: real 0m3.939s real 0m4.098s ~53% decrease in bootup time, seems legit. ### Potential risks * This gem does affect how files are loaded during app boot, so there is some risk that app boot in a non-dev environment goes awry. A test deploy should alleviate those concerns. * It's possible that bootsnap negatively affects development by caching filepaths too aggressively (see 'stable entries' under https://github.com/Shopify/bootsnap#path-pre-scanning). I don't know of a use case where those paths need to be uncached. I'm adding this gem to every group, since there's nothing development-specific about it and it seems better to both take advantage of speed gains in other envs and run as similar of a setup in dev and prod as possible. Shopify uses this gem in every environment and it's the new Rails default. Tangentially, I'm also curious if we've tried upgrading to Rails 5.2 outright. ### Acceptance Criteria - [x] Tests pass ### Testing Plan In addition to working with this gem enabled locally for development, I'd like to do a test deploy to an AWS environment and restart to confirm that both of these succeed.
This adds bootsnap to newly-generated projects, which generally reduces application boot times by over 50%.
I heard through the grapevine that people would be open to this.
Early releases of bootsnap only supported macOS, but recent versions now additionally support linux and (at least in theory) most *BSDs. Windows support is still not implemented.
This change is small, but not trivial, and it's been a long time since I've been in this codebase. I'm looking for guidance on how best to get this change documented, tested, and merged.
cc @rafaelfranca