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

Accelerate asset delivery in Rails 4, yay or nay? #10291

Closed
SamSaffron opened this issue Apr 22, 2013 · 29 comments

Comments

@SamSaffron
Copy link
Contributor

commented Apr 22, 2013

At Discourse we unfortunately have an enormous amount of assets to deliver in development mode.

Current count is 373 css and js files.

This means that clicking the "reload" button in the browser in development mode results in 800 requests to the web server.

Other big "asset heavy" apps (like ember js etc) tend to have lots of js assets. This is also discussed here: http://discuss.emberjs.com/t/what-are-people-doing-about-the-gazzillion-file-problem/1006


The main reason asset delivery is slow in Rails dev environment is that the entire middleware stack needs to be walked prior to reaching sprockets and determining if an asset changed.

For context, this is Discourse prior to improving this:

image

Introducing this middleware in front of the stack:

module Middleware
  # this class cheats and bypasses rails altogether if the client attempts
  # to download a static asset
  class TurboDev
    def initialize(app, settings={})
      @app = app
    end
    def call(env)
      # hack to bypass all middleware if serving assets, a lot faster 4.5 seconds -> 1.5 seconds
      if (etag = env['HTTP_IF_NONE_MATCH']) && env['REQUEST_PATH'] =~ /^\/assets\//
        name = $'
        etag = etag.gsub "\"", ""
        asset = Rails.application.assets.find_asset(name)
        if asset && asset.digest == etag
          return [304,{},[]]
        end
      end

      @app.call(env)
    end
  end
end

Takes us to:

image

So in this test run that is over 4 seconds down to 1.3 seconds.


I discussed this with @josh over twitter, the current thinking is that sprockets 3 is going to use source maps only for dev, bypassing much of this issue.

My concern is that sourcemaps may introduce extra server delays and outlaw some optimisations you can do with split up files. For example, when assets are split you could use $LAB to load the js files, circumventing the browser refresh forced round trip.


Anyway, we need to decide if we want this optimisation or not in Rails 4.

@guilleiguaran

This comment has been minimized.

Copy link
Member

commented Apr 22, 2013

Note that if this is accepted it should be add to sprockets-rails, but lets keep the discussion about it here.

I'm 👍 on this, looks fine for me

@josevalim

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2013

Does Rails.application.assets.find_asset(name) compile the asset if one is not there? Or it only looks up a cached one?

If it can compile an asset

We can have issues because assets compilation may refer to a constant in a model. However, there is a chance this is not an issue due to the reloading mechanism from Rails 3.1 (or Rails 3.2). If this is the case, it is worth considering to move the WHOLE assets compilation away from the router and to somewhere upper in the middleware stack. Maybe after Rack::Runtime? This would also solve the logging issue.

If it only looks up a cached one

I don't have anything to complain.

@SamSaffron

This comment has been minimized.

Copy link
Contributor Author

commented Apr 22, 2013

@jose I thought this funnels into sprockets and is a simple hash lookup,
need to review and confirm though

On Mon, Apr 22, 2013 at 2:21 PM, José Valim notifications@github.comwrote:

Does Rails.application.assets.find_asset(name) compile the asset if one
is not there? Or it only looks up a cached one?

If it can compile an asset

We can have issues because assets compilation may refer to a constant in a
model. However, there is a chance this is not an issue due to the reloading
mechanism from Rails 3.1 (or Rails 3.2). If this is the case, it is worth
considering to move the WHOLE assets compilation away from the router and
to somewhere upper in the middleware stack. Maybe after Rack::Runtime?

If it only looks up a cached one

I don't have anything to complain.


Reply to this email directly or view it on GitHubhttps://github.com//issues/10291#issuecomment-16753407
.

@josh

This comment has been minimized.

Copy link
Member

commented Apr 22, 2013

👎 from me

On Sun, Apr 21, 2013 at 11:04 PM, Guillermo Iguaran
notifications@github.com wrote:

Note that if this is accepted it should be in sprockets-rails, but lets keep the discussion about it here.

I'm 👍 on this, looks fine for me

Reply to this email directly or view it on GitHub:
#10291 (comment)

@pixeltrix

This comment has been minimized.

Copy link
Member

commented Apr 22, 2013

@josh care to expand on the reasons for the 👎 ?

@SamSaffron

This comment has been minimized.

Copy link
Contributor Author

commented Apr 22, 2013

@josh I take it the reason is source maps, is this going to be feasible for Rails 4 launch?

@josh

This comment has been minimized.

Copy link
Member

commented Apr 22, 2013

First, I don't when the problem is Rails has too many layers of middleware call stacks, the fix is to add another middleware. This basically duplicates sprockets' server functionality just in middleware.

Also as mentioned, the debug split out will be gone in Sprockets 3. You won't have that many requests.

With 800+ assets, I think you are hitting a Ruby problem. Having the alternate style setup of writing to a static file on save, then having nginx (or whatever frontend) serving those files locally is going to be insanely fast. You can still be using Sprockets compilation tech, but maybe using something like Guardfile to persist those static files.

@josevalim

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2013

@josh what about just moving it away from the router and up in the middleware stack? If there any reason not to? Would we miss anything?

@josh

This comment has been minimized.

Copy link
Member

commented Apr 22, 2013

It'd be nice to know what middleware is slow. I think entire concept of hitting the Rack interface is always going to be to slow for @SamSaffron.

btw, this is all a big change for already being rc4 or whatever.

@rafaelfranca

This comment has been minimized.

Copy link
Member

commented Apr 22, 2013

Agree with @josh I don't want to add this to Rails 4. We are planning to release the rc soon.

@josh

This comment has been minimized.

Copy link
Member

commented Apr 22, 2013

I think if you really want to opt into this approach, you can already hoist the sprockets server up the stack yourself. Even before it touches that slow ass Rails!

# config.ru
map "/assets" -> { run Rails.application.assets }
run Foo::Application
@SamSaffron

This comment has been minimized.

Copy link
Contributor Author

commented Apr 22, 2013

@josh I worry that various asset generation may depend on middleware. For example tilt templates that require db access, if they are hoisted in front we are probably going to get annoying connection leaks in dev.

The proposed design here leaves asset validation in front, but generation in its tested spot.

Its not that any middleware is particularly super slow, its just that by the time we reach the router and walked the 50 omniauth frames of doom, initialised current user and opened our db connection, we are already 5-10ms in.

Keep in mind, this change is only about speeding dev for asset heavy rails apps and the unavoidable trend now is creating these asset heavy apps.

My big concern is that rails 4 will ship and we will only see the sprockets upgrade in 2014 leaving ember/angular and backbone with suboptimal dev experiences, historically rails seems very judicious about accepting new sprocket versions.

@airhorns

This comment has been minimized.

Copy link
Contributor

commented May 15, 2013

I'd like to +1 this from Shopify as well, we were forced to stop splitting files in dev and return a 20000 line file because the request overhead is too high as @SamSaffron has found.

@sudhirj

This comment has been minimized.

Copy link

commented Jul 3, 2013

It might be easier to just write this up as a gem for now, and let people include it in their development group.

@sricc

This comment has been minimized.

Copy link

commented Jul 19, 2013

+1

@hrdwdmrbl

This comment has been minimized.

Copy link

commented Oct 10, 2013

@SamSaffron OMG, please! I just heard you mention this work on the RR podcast. I'm nearing ~350 js files in a backbone application and load times are insane. I spent a couple days optimizing the way rails writes script tags, but this was the other slow part for me that I wasn't able to optimize.

@senny senny added the discussion label Feb 4, 2014

@SamSaffron

This comment has been minimized.

Copy link
Contributor Author

commented Feb 11, 2014

@rafaelfranca should we add this to master?

sprockets is no closer to getting source maps and this problem is of high impact to many current rails apps in dev mode.

@rafaelfranca

This comment has been minimized.

Copy link
Member

commented Feb 11, 2014

@SamSaffron I think we should not include a half-baked solutions in Rails. I know it is currently a problem but I think we should do two things:

  1. Solve this problem in the right way. This mean adding source maps do sprockets
  2. Release a dirty fix as a gem.
@bf4

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2014

+1 to anything that raises visibility of the issue and solutions.. even if they link back here

@rafaelfranca rafaelfranca added this to the 4.2.0 milestone Feb 12, 2014

@sheerun

This comment has been minimized.

Copy link
Contributor

commented Feb 24, 2014

I think this is not rails responsibility

@bf4

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2014

@sheerun fwiw, Rails has a very fuzzy sense of what is it's responsibility. Perhaps this can be an optional middleware included in Rails? And/or written up in the guides?

@sheerun

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2014

I think middleware, and including it in configuration would be far better than code in Rails and enabling it in configuration.

@rafaelfranca

This comment has been minimized.

Copy link
Member

commented Apr 1, 2014

I'm closing this issue since I believe this is not the best fix.

I'll work to get source maps included on Rails for 4.2

@SamSaffron

This comment has been minimized.

Copy link
Contributor Author

commented Apr 1, 2014

One thing to keep in mind here is that cost of changing an asset will
increase with a source map based approach.

Say you have 1MB of JavaScript, you make a change to a 1K file. With source
map approach you are forced to

  1. Concatenate everything
  2. Browser must re-parse entire js kaboodle
  3. And most seriously, a source map needs to be re-generated it may get
    tricky

Not saying its impossible, but not a trivial problem to solve.

On Tue, Apr 1, 2014 at 1:13 PM, Rafael Mendonça França <
notifications@github.com> wrote:

Closed #10291 #10291.

Reply to this email directly or view it on GitHubhttps://github.com//issues/10291
.

@dpritchett

This comment has been minimized.

Copy link

commented Apr 21, 2014

See @markijbema's gem packaging of the quick and dirty solution proposed by @SamSaffron above: https://github.com/markijbema/turbo_dev_assets

markijbema added a commit to Factlink/factlink-core that referenced this issue May 14, 2014
@chancancode

This comment has been minimized.

Copy link
Member

commented Feb 6, 2015

Been investigating this just this week, wanted to leave a few notes here for anyone using this.

I thought this funnels into sprockets and is a simple hash lookup

Upon further investigation, this is actually not true – find_asset is designed to return fresh assets on every call, so if the asset is stale, it would actually block and eventually re-compile the asset file (and any dependencies).

Therefore, if you are worried about...

I worry that various asset generation may depend on middleware. For example tilt templates that require db access, if they are hoisted in front we are probably going to get annoying connection leaks in dev.

The proposed design here leaves asset validation in front, but generation in its tested spot.

...it is already be a problem here, since the asset is actually re-compiled at the Rails.application.assets.find_asset(name) call, inside this middleware.

So, you should be aware that depends on where this middleware is injected and what crazy things you do in your asset files, you might be bypassing things like Active Record's connection manager and Rail's Reloader, so if you ended up requesting a stale asset via this middleware, it could result in memory/connection leaks.

In practice, it is probably quite rare for this middleware to encounter a stale asset file, as the asset has probably been compiled in the previous request (the HTML page).

When you use the javascript_include_tag helper, Rails will internally call find_asset, so any stale asset would be compiled at that point (which is necessary to generate either the expanded list of script tags or the bundle digest, which depends on the digest of all its dependencies and more). So, your subsequent request to actually fetch the src in the <script> tag will very likely result in fetching a fresh entry from the asset cache. Therefore, under normal use, the proposed middleware will probably never see a stale asset.

However, if your browser is somehow requesting assets that were not generated from javascript_include_tag (e.g. you requested/refreshed an asset file in your browser without refreshing the HTML page, or perhaps you have some strange AMD setup), or if your asset somehow changed after the HTML page was returned but before your browser requested the asset, then you might have a problem.

In other words – this middleware offers no extra protection over just using Sprockets::Server, and it is missing some essential functionality such as error handling and differentiating between bundle and "bare" asset requests (applications.js vs application.js?body=1).

So if you are not worried about the edge cases, you are indeed better off doing something like map "/assets" -> { run Rails.application.assets } as @josh suggested, instead of using this middleware.

On the other hand, if you are worried about the edge cases, then the middleware should look something like this (base on the latest rev from discourse:

module Middleware

  # Cheat and bypass Rails in development mode if the client attempts to download a static asset
  # that's already been downloaded.
  #
  # Also ensures that assets are not cached in development mode. Around Chrome 29, the behavior
  # of `must-revalidate` changed and would often not request assets that had changed.
  #
  #  To use, include in your project and add the following to development.rb:
  #
  #  require 'middleware/turbo_dev'
  #  config.middleware.insert 0, Middleware::TurboDev
  #
  class TurboDev
    def initialize(app, settings={})
      @app = app
    end

    def call(env)
      # hack to bypass all middleware if serving assets, a lot faster 4.5 seconds -> 1.5 seconds
      if (request_etag = env['HTTP_IF_NONE_MATCH']) && (asset_path = asset_path_for(env['REQUEST_PATH'])
        bundle = is_bundle?(env['QUERY_STRING'])
        asset  = find_fresh_asset(asset_path, bundle)

        if asset && request_etag == etag(asset)
          return [ 304, {}, [] ]
        end
      end

      status, headers, response = @app.call(env)
      headers['Cache-Control'] = 'no-cache' if asset_path
      [status, headers, response]
    end

    private

      IS_ASSET = /^\/assets\/(?<path>.+)$/

      def asset_path_for(request_path)
        IS_ASSET.match(request_path.to_s).try(:[], :path)
      end

      IS_BUNDLE = /body=(1|t)/

      def is_bundle?(query_string)
        query_string.to_s =~ IS_BUNDLE
      end

      def find_fresh_asset(path, bundle)
        Rails.application.assets.instance_eval do
          if (asset = @assets[cache_key_for(path, bundle: !!bundle)]) && asset.fresh?(self)
            return asset
          end
        end
      end

      def etag(asset)
        %("#{asset.digest}")
      end
  end

end

...so, not so simple anymore. And of course this would totally break with sprockets > 2.x. And it still doesn't do a lot of the stuff that sprockets does for you, like handling fingerprinted asset paths.

By the way, if find_fresh_asset returns something, then you might as well serve the content right from the middleware to make things even faster. But then of course, you would have to make sure you set the essential headers too, like "Content-Type", "Content-Length", "Cache-Control" and "ETag". But then if you do all of that, it will start to look awfully like Sprockets::Server.

Conclusion

In hindsight, ...

  1. @josh is completely right. The proposed middleware does not offer anything over just hoisting Sprockets::Server to the top.
  2. @rafaelfranca made the right call – in that hoisting it to the top could be problematic and we shouldn't bother with that by default (most apps won't have this problem).
  3. If you have a lot of assets and your app is really slow, and you know what you are doing, this might be fair trade-off to make, since the edge cases and not that common. However, you should probably just use Sprockets::Server in that case.

cc @SamSaffron

@SamSaffron

This comment has been minimized.

Copy link
Contributor Author

commented Feb 9, 2015

@chancancode very good point about find_asset, I misread that and can confirm that its not really doing what I initially thought it was doing.

I do however believe this is a severe issue that a lot are facing, and think there is a very simple solution to this problem with very minor changes on rails side.

Since "find_asset" already has digests why not append them to the asset name, eg in development you would have:

discourse/lib/some-file.js?body=1&digest=abcabc123123

Then we can simply set these assets to never expire, which allows for people to run nginx in front or rack cache in front and cache server side forever if they wish. They will also get a more reasonable reload experience without any magic. Rails can consider then adding rack::cache or something similar to that in development so there is a turnkey solution that everyone get.

This seems like a reasonable solution to this problem and would add zero additional cost on rails side.

I do not think sourcemaps will solve this issue. regenerating them on single asset invalidation and building up bundles is going to add up a lot on dev.

cc @josh

@mib32

This comment has been minimized.

Copy link

commented Jul 18, 2015

I installed TurboDevAssets middleware, but my asset get log looks like this - https://gist.github.com/mib32/e7332248a1a5cf0444d3

I use Rails 4.2.3 and according to log, first asset is served at 14:17:27 and last at 14:17:49 which gives us 22 seconds, so i believe that means that this middleware no longer actual for Rails 4.2.3?

@mib32

This comment has been minimized.

Copy link

commented Jul 18, 2015

Seems like i need to take a look at #18828, thanks.

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