Skip to content
This repository has been archived by the owner. It is now read-only.
Permalink
Browse files

Stop gzipping manifest by default

  • Loading branch information...
josh committed Jun 21, 2014
1 parent 784a93d commit d388ef7cc0e36e710539fb11799d7e5518609d61
Showing with 2 additions and 30 deletions.
  1. +2 −16 lib/sprockets/manifest.rb
  2. +0 −14 test/test_manifest.rb
@@ -178,8 +178,6 @@ def compile(*args)
assets[asset.logical_path] = asset.digest_path

target = File.join(dir, asset.digest_path)
mime_type = environment.mime_types[asset.content_type]
gzip = mime_type && mime_type[:charset]

if File.exist?(target)
logger.debug "Skipping #{target}, already exists"
@@ -188,16 +186,6 @@ def compile(*args)
asset.write_to target
end

gziped_target = "#{target}.gz"

if File.exist?(gziped_target)
logger.debug "Skipping #{gziped_target}, already exists"
elsif gzip
logger.info "Writing #{gziped_target}"
gziped_asset = find_asset(filename, 'gzip')
gziped_asset.write_to gziped_target
end

filenames << filename
end
end
@@ -213,7 +201,6 @@ def compile(*args)
#
def remove(filename)
path = File.join(dir, filename)
gzip = "#{path}.gz"
logical_path = files[filename]['logical_path']

if assets[logical_path] == filename
@@ -222,7 +209,6 @@ def remove(filename)

files.delete(filename)
FileUtils.rm(path) if File.exist?(path)
FileUtils.rm(gzip) if File.exist?(gzip)

save

@@ -270,10 +256,10 @@ def backups_for(logical_path)
end

# Basic wrapper around Environment#find_asset. Logs compile time.
def find_asset(logical_path, encoding = nil)
def find_asset(logical_path)
asset = nil
start = Utils.benchmark_start
asset = environment.find_asset(logical_path, accept_encoding: encoding)
asset = environment.find_asset(logical_path)
logger.debug do
ms = "(#{Utils.benchmark_end(start)}ms)"
"Compiled #{logical_path} #{ms}"
@@ -173,18 +173,6 @@ def teardown
assert_equal gallery_digest_path, data['assets']['gallery.css']
end

test "compress assets" do
gallery_digest_path = @env['gallery.css'].digest_path
app_digest_path = @env['application.js'].digest_path
img_digest_path = @env['blank.gif'].digest_path

@manifest.compile('gallery.css', 'application.js', 'blank.gif')

assert File.exist?("#{@dir}/#{gallery_digest_path}.gz")
assert File.exist?("#{@dir}/#{app_digest_path}.gz")
assert !File.exist?("#{@dir}/#{img_digest_path}.gz")
end

test "recompile asset" do
digest_path = @env['application.js'].digest_path
filename = fixture_path('default/application.js.coffee')
@@ -224,7 +212,6 @@ def teardown

@manifest.compile('application.js')
assert File.exist?("#{@dir}/#{digest_path}")
assert File.exist?("#{@dir}/#{digest_path}.gz")

data = JSON.parse(File.read(@manifest.filename))
assert data['files'][digest_path]
@@ -233,7 +220,6 @@ def teardown
@manifest.remove(digest_path)

assert !File.exist?("#{@dir}/#{digest_path}")
assert !File.exist?("#{@dir}/#{digest_path}.gz")

data = JSON.parse(File.read(@manifest.filename))
assert !data['files'][digest_path]

13 comments on commit d388ef7

@amw

This comment has been minimized.

Copy link

replied Sep 5, 2014

Is gzipping assets going away from spockets entirely? I was wondering in this issue in sprockets-rails: rails/sprockets-rails#160.

@josh

This comment has been minimized.

Copy link
Contributor Author

replied Sep 5, 2014

Its an interesting tradeoff. The default rake task .gz stuff will be completely gone, however, there will be more "first class" support for generating gzip'd assets. You'll even be able to override the gzip implementation to use whatever alt compression format you want. environment.find("application.js", accept_encoding: "gzip") will return a gzip'd application.js asset. You can then write that to whatever and apache, nginx config is on you to figure out.

@amw

This comment has been minimized.

Copy link

replied Sep 5, 2014

But what will be calling that? Do you suggest this should go back to sprockets-rails or every individual Rails app should implement that on its own?

@fxn

This comment has been minimized.

Copy link
Contributor

replied May 14, 2015

I have seen the linked article in the CHANGELOG as rationale for this removal. That's why @vijaydev and I have always rejected Apache configurations in the assets pipeline guide, we just weren't certain those were really robust.

But NGINX has gzip_static, which was the motivation for writing this feature (I wrote it) and as far as I know it works correctly. I think a flag would have been better because now NGINX users are left without the ability of having this in a normal workflow.

@fxn

This comment has been minimized.

Copy link
Contributor

replied May 14, 2015

For the archives, I've defined this Capistrano task by now:

namespace :deploy do
  after :normalize_assets, :gzip_assets do
    on release_roles(fetch(:assets_roles)) do
      assets_path = release_path.join('public', fetch(:assets_prefix))
      within assets_path do
        execute :find, ". \\( -name '*.js' -o -name '*.css' \\) -print0 | xargs -0 gzip --keep --best --quiet --force"
      end
    end
  end
end

Not battle tested, but seems to be correct.

@josh if you'd consider taking this back with a flag (default to false if you want), I'd volunteer of course.

@amw

This comment has been minimized.

Copy link

replied May 14, 2015

Here's what I've been using. It depends on parallel gem to use multiple CPU cores, but it can easily be simplified to do everything on the main thread.

# lib/tasks/assets.rake
namespace :assets do
  ManifestPattern = /^manifest-[0-9a-f]{32}\.json$/
  ExtensionsToCompress = %w{.js .css .json}.freeze

  desc "GZip assets"
  task :compress do
    files = Pathname.glob(Rails.root.join('public/assets/**/*')).map do |file|
      next unless file.extname.in? ExtensionsToCompress
      next if file.basename.to_s =~ ManifestPattern

      gzipped = file.to_s + ".gz"

      next if File.exist? gzipped

      [file, gzipped]
    end.compact

    start = -> item, index {
      gzipped = item.last
      puts "Creating #{gzipped}"
    }

    Parallel.each files, start: start do |file,gzipped|
      Zlib::GzipWriter.open gzipped, Zlib::BEST_COMPRESSION do |dest_io|
        open file, "rb" do |source_io|
          dest_io << source_io.read(16384) until source_io.eof?
        end
      end
    end
  end
end
# lib/capistrano/tasks/assets.rake
namespace :deploy do
  desc "Gzip assets"
  task :compress_assets do
    on roles(fetch(:assets_roles)) do
      within release_path do
        with rails_env: fetch(:rails_env) do
          execute :rake, "assets:compress"
        end
      end
    end
  end
end

after "deploy:compile_assets", "deploy:compress_assets"
@josh

This comment has been minimized.

Copy link
Contributor Author

replied May 15, 2015

nginx's mod_gzip does this automatically and uses transfer-encoding correctly (vs mod_gzip_static).

I don't think this should be in sprockets core anymore.

@tobernguyen

This comment has been minimized.

Copy link

replied May 16, 2015

thanks @amw. Your solution works great.

@amw

This comment has been minimized.

Copy link

replied May 16, 2015

@josh Chunked Transfer-Encoding is not needed for static pre-gzipped files, because server immediately knows the size of the compressed file and can fire simpler Content-Length header immediately.

mod_gzip_static works very well for me. It 1) compresses each file only once, 2) can use better compression, 3) and makes the compressed version immediately available when the request comes.

Using my example application.js file.

$ls -l application*js*
-rw-rw-r--  1 production production 504779 May  6 10:59 application-6be22e774363e55425d2ef3c62389933.js
-rw-rw-r--  1 production production 141028 May  6 10:59 application-6be22e774363e55425d2ef3c62389933.js.gz

Response headers

Cache-Control: max-age=315360000, public
Connection: keep-alive
Content-Encoding: gzip
Content-Length: 141028
Content-Type: application/javascript
Date: Sat, 16 May 2015 10:45:00 GMT
Expires: Thu, 31 Dec 2037 23:55:55 GMT
Server: nginx/1.6.2

Notice the Content-Length exactly matches the compressed file on disk. Now to compare with file created using gzip -1 where 1 is the default compression level of mod_gzip. You can change the option, but it taxes CPU. It's a default for a reason.

-rw-rw-r-- 1 production production 171238 May 16 10:53 test-gzip.js.gz

That's a 21% larger transfer. And it's being started after a larger delay too.

@fxn

This comment has been minimized.

Copy link
Contributor

replied May 20, 2015

@josh ngx_http_gzip_module does not do this. I am sure you know it and that was a sentence just quickly written, but for the archives what the standard compression modules do is compress on-the-fly. On the other hand ngx_http_gzip_static_module is able to serve an already compressed file if present on disk.

I know two use cases for ngx_http_gzip_static_module:

  1. As with other asset optimization techniques, if you are able to compress text-based assets as part of your deploy workflow then you can apply maximum compression because it is going to be done only once and can afford the extra CPU cycles, and later the server has to do nothing on-the-fly, just serve the compressed variant from disk.
  2. Serve compressed assets from CDNs that do not support compression on-the-fly for browsers that understand compressed assets.

Regarding Transfer-Encoding, we could argue if theoretically this should be done at that level. But at the practical level that header is useless for the majority of use cases of Sprockets.

The pair Accept-Encoding/Content-Encoding is absolutely dominant and it is what we have to deploy for today. It is also an example used in the HTTP spec itself.

On the other hand, in order to be able to indicate compression in Transfer-Encoding the client has to first acknowledge it is willing to accept it via the TE header. As of this writing Safari, Firefox, and Chrome send Accept-Encoding: gzip and do not send any TE header, so as a matter of fact the server just can't compress via Transfer-Encoding for them. In addition to that, it seems Chrome is not even considering TE for compression. So Transfer-Encoding for compression is useless for all practical purposes.

In my experience ngx_http_gzip_static_module works flawlessly, have used it for years. And that is what motivated me to add this feature back in 2011.

That is only a response to the objection to the whole idea of using this technique. Of course, whether this belongs in Sprockets core or not is your call.

@fxn

This comment has been minimized.

Copy link
Contributor

replied May 20, 2015

For the archives, I have updated the one-liner to skip already compressed files, and fork 8 processes. Now maintaining a gist with the snippet.

@amw I think the task should run after :normalize_assets because the NGINX module recommends the mtime of the compressed and uncompressed files to be the same. In my one-liner I delegate that to gzip(1), because it automatically sets the mtime that way.

@schneems

This comment has been minimized.

Copy link
Contributor

replied Dec 2, 2015

@fxn can you point me at the nginx docs talking about mtime with gzip?

@fxn

This comment has been minimized.

Copy link
Contributor

replied Dec 2, 2015

@schneems it is a recommendation in the docs of gzip_static:

It is recommended that the modification date and time of original and compressed files be the same.

I don't know the rationale (we could write to the module authors maybe), but I suspect it could be related to sending the same caching metadata regardeless of compression.

Please sign in to comment.
You can’t perform that action at this time.