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

Cache public/assets if rake assets:clean_expired is available #44

Closed

Conversation

ndbroadbent
Copy link

This pull request provides support for the turbo-sprockets-rails3 gem for Rails 3 apps.

If a assets:clean_expired Rake task is available, it will remove expired assets and cache them for the next run.

@@ -43,6 +43,9 @@ def run_assets_precompile_rake_task
if File.exists?("public/assets/manifest.yml")
puts "Detected manifest.yml, assuming assets were compiled locally"
else
FileUtils.mkdir_p('public')
cache_load "public/assets"
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this doesn't need to be conditional - It will just do nothing if 'public/assets' isn't cached.


# If `turbo-sprockets-rails3` is available, remove old assets and
# cache them for next time.
if File.read('Gemfile.lock').include?('turbo-sprockets-rails3')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of grepping the Gemfile.lock you can use rake_task_defined? method. Then it doesn't depend on the source of the rake task.

@ndbroadbent
Copy link
Author

Thanks, I've pushed both of those changes

@ndbroadbent
Copy link
Author

Using rake_task_defined?('assets:clean_expired') adds an extra delay of around 10 seconds to all deploys, so I'm not sure if that's better than just testing the Gemfile.lock.

I don't know if any other gems would have any reason to provide an assets:clean_expired task for Rails 3. And if the task was defined in the app itself, I'm not sure why they wouldn't be using my gem :)

@ndbroadbent
Copy link
Author

I'm going to move conversation over to that pull request. Do you have any benchmarks or anyone using your fork?

Richard Schneeman

Yep, I've received reports that people have used my heroku buildpack successfully with their apps. Here's some benchmarks for the turbo-sprockets-rails3 gem:

Before / After

Here's some asset compilation times for a smallish Rails app.
(running time RAILS_ENV=production RAILS_GROUPS=assets rake assets:precompile)

Before:

asset:precompile: 26.993s

After:

Task Time
Initial asset:precompile 18.525s
assets:precompile:primary 9.772s
assets:precompile:nondigest 64ms
Unchanged assets 9.386s
assets:precompile:primary 877ms
assets:precompile:nondigest 91ms
One changed application.js dependency 12.386s
assets:precompile:primary 3.910s
assets:precompile:nondigest 48ms

@schneems
Copy link
Contributor

schneems commented Nov 5, 2012

Ok, to make sure i'm reading this right you're saying that with no turbo sprockets we are at 26.993 seconds, with turbo-sprockets and no changes we are down to 18.525 seconds, using cached assets we are down to 9.386 seconds (here most of the time comes from detecting the rake task?). Then if we change something and re-compile we're back up to 12 seconds.

It was pointed out to me that there is actually an official method for detecting the presence of a gem in the Gemfile using the gem_is_bundled? method, if it really does take a significant amount of time to check for that rake task (and it seems like it does) perhaps looking for the gem is best.

@ndbroadbent
Copy link
Author

Those benchmarks were actually on a plain Rails app, so most of the 9.386 seconds was just preparing the environment for the task to run. Actually checking for changes only took something like 100ms.

So when rake_task_defined? starts up the environment to check for a assets:clean_expired task, it's adding a further 9 seconds. We could use gem_is_bundled?, but another idea would be to just attempt to run rake assets:clean_expired without the dry run, and then cache the assets if it the task succeeded.

@ndbroadbent
Copy link
Author

Have updated the pull request with the suggestion above. But it will still make deployments a little slower for people who aren't using the gem

@ndbroadbent
Copy link
Author

P.S. The benchmark above was for a pretty small Rails app. I've received emails saying that it cut down their total deploy time from 15 minutes to 2.

@jeyb
Copy link

jeyb commented Nov 6, 2012

We have used this in production for a much larger application. Initially asset compilation took around 600s, since using this, it has been reduced to 30s. Worth noting we don't use Node to compile, we use therubyracer as it's quite a bit faster for us.

@ndbroadbent
Copy link
Author

Have updated the PR to detect the turbo-sprockets-rails3 gem using gem_is_bundled?. Apps not using the gem won't experience any extra delays due to the previous rake task detection.

Any cached assets will be deleted if the gem is ever removed, or if the assets:clean_expired task fails.

@schneems
Copy link
Contributor

Hey, just an update. I'm doing some testing with your buildpack fork, I'll let you know how it goes.

@schneems
Copy link
Contributor

I'm giving this my 👍 in testing it really cut down on time and didn't seem to have any adverse effects on projects without the gem. Thanks again for your work on the gem and on this PR. Sorry we've been a bit slow, but i'm quite happy with the end result. Now just need the final once over by @hone

@mcolyer
Copy link

mcolyer commented Dec 20, 2012

I'd love to see this integrated.

@maletor
Copy link

maletor commented Jan 17, 2013

ping @hone

@maletor
Copy link

maletor commented Mar 21, 2013

Bumpity bumpity bump bump.

@sbleon
Copy link
Contributor

sbleon commented Apr 9, 2013

👍

This speeds asset compilation time dramatically on my apps.

@flomotlik
Copy link

Any update on this? @hone or anyone from @heroku got a word for us?

@maletor
Copy link

maletor commented Jun 7, 2013

Hello? Still waiting on this one.

@jvdp
Copy link

jvdp commented Jun 12, 2013

+1 for faster deploys!

@borski
Copy link

borski commented Jun 12, 2013

👍

@PikachuEXE
Copy link

This is not pulled yet??

@jeyb
Copy link

jeyb commented Jan 21, 2014

I don't think this merges cleanly with master anymore.

@schneems schneems closed this Feb 27, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants