From 77b278b79e1a8ac6d2e4541fb0c6b51f552c23bc Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Thu, 29 Jun 2017 01:59:51 +0000 Subject: deflater: rely on frozen_string_literal This improves readability of our code and can allow us to generate less garbage in Ruby 2.3+ for places where we didn't already use constants. We can also avoid the old constant lookups (and associated inline cache overhead) this way. --- lib/rack/deflater.rb | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/lib/rack/deflater.rb b/lib/rack/deflater.rb index 46d5b20a..9b798bab 100644 --- a/lib/rack/deflater.rb +++ b/lib/rack/deflater.rb @@ -1,3 +1,4 @@ +# frozen_string_literal: true require "zlib" require "time" # for Time.httpdate require 'rack/utils' @@ -52,7 +53,7 @@ module Rack case encoding when "gzip" headers['Content-Encoding'] = "gzip" - headers.delete(CONTENT_LENGTH) + headers.delete('Content-Length') mtime = headers.key?("Last-Modified") ? Time.httpdate(headers["Last-Modified"]) : Time.now [status, headers, GzipStream.new(body, mtime)] @@ -61,7 +62,7 @@ module Rack when nil message = "An acceptable encoding for the requested resource #{request.fullpath} could not be found." bp = Rack::BodyProxy.new([message]) { body.close if body.respond_to?(:close) } - [406, {CONTENT_TYPE => "text/plain", CONTENT_LENGTH => message.length.to_s}, bp] + [406, {'Content-Type' => "text/plain", 'Content-Length' => message.length.to_s}, bp] end end @@ -102,13 +103,13 @@ module Rack # Skip compressing empty entity body responses and responses with # no-transform set. if Utils::STATUS_WITH_NO_ENTITY_BODY.include?(status) || - headers[CACHE_CONTROL].to_s =~ /\bno-transform\b/ || + headers['Cache-Control'].to_s =~ /\bno-transform\b/ || (headers['Content-Encoding'] && headers['Content-Encoding'] !~ /\bidentity\b/) return false end # Skip if @compressible_types are given and does not include request's content type - return false if @compressible_types && !(headers.has_key?(CONTENT_TYPE) && @compressible_types.include?(headers[CONTENT_TYPE][/[^;]*/])) + return false if @compressible_types && !(headers.has_key?('Content-Type') && @compressible_types.include?(headers['Content-Type'][/[^;]*/])) # Skip if @condition lambda is given and evaluates to false return false if @condition && !@condition.call(env, status, headers, body) -- cgit v1.2.3-24-ge0c7 From 1b94e07f8600dd65a8ba1970161e97e32690e05c Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Thu, 29 Jun 2017 01:59:51 +0000 Subject: deflater: avoid wasting an ivar slot on @closed We can merely set the @body to nil ensure we do not call close on the @body, twice. Saving an ivar slot can save 8 bytes per object at minimum, and this makes me feel more comfortable about using another ivar for the next commit. --- lib/rack/deflater.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/rack/deflater.rb b/lib/rack/deflater.rb index 9b798bab..d575adfe 100644 --- a/lib/rack/deflater.rb +++ b/lib/rack/deflater.rb @@ -70,7 +70,6 @@ module Rack def initialize(body, mtime) @body = body @mtime = mtime - @closed = false end def each(&block) @@ -91,9 +90,8 @@ module Rack end def close - return if @closed - @closed = true @body.close if @body.respond_to?(:close) + @body = nil end end -- cgit v1.2.3-24-ge0c7 From 181e56e011f4c321895bfd01f20cccb3ec1cafa5 Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Thu, 29 Jun 2017 01:59:51 +0000 Subject: deflater: support "sync: false" option Flushing after after very flush is great for real-time apps. However, flushing is inefficient when apps use Rack::Response to generate many small writes (e.g. Rack::Lobster). Allow users to disable the default "sync: true" behavior to reduce bandwidth usage, otherwise using Rack::Deflater can lead to using more bandwidth than without it. --- lib/rack/deflater.rb | 11 ++++++++--- test/spec_deflater.rb | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 3 deletions(-) diff --git a/lib/rack/deflater.rb b/lib/rack/deflater.rb index d575adfe..821f708b 100644 --- a/lib/rack/deflater.rb +++ b/lib/rack/deflater.rb @@ -24,11 +24,15 @@ module Rack # 'if' - a lambda enabling / disabling deflation based on returned boolean value # e.g use Rack::Deflater, :if => lambda { |env, status, headers, body| body.map(&:bytesize).reduce(0, :+) > 512 } # 'include' - a list of content types that should be compressed + # 'sync' - Flushing after every chunk reduces latency for + # time-sensitive streaming applications, but hurts + # compression and throughput. Defaults to `true'. def initialize(app, options = {}) @app = app @condition = options[:if] @compressible_types = options[:include] + @sync = options[:sync] == false ? false : true end def call(env) @@ -56,7 +60,7 @@ module Rack headers.delete('Content-Length') mtime = headers.key?("Last-Modified") ? Time.httpdate(headers["Last-Modified"]) : Time.now - [status, headers, GzipStream.new(body, mtime)] + [status, headers, GzipStream.new(body, mtime, @sync)] when "identity" [status, headers, body] when nil @@ -67,7 +71,8 @@ module Rack end class GzipStream - def initialize(body, mtime) + def initialize(body, mtime, sync) + @sync = sync @body = body @mtime = mtime end @@ -78,7 +83,7 @@ module Rack gzip.mtime = @mtime @body.each { |part| gzip.write(part) - gzip.flush + gzip.flush if @sync } ensure gzip.close diff --git a/test/spec_deflater.rb b/test/spec_deflater.rb index 0f27c859..410a1438 100644 --- a/test/spec_deflater.rb +++ b/test/spec_deflater.rb @@ -372,4 +372,38 @@ describe Rack::Deflater do verify(200, response, 'gzip', options) end + + it 'will honor sync: false to avoid unnecessary flushing' do + app_body = Object.new + class << app_body + def each + (0..20).each { |i| yield "hello\n".freeze } + end + end + + options = { + 'deflater_options' => { :sync => false }, + 'app_body' => app_body, + 'skip_body_verify' => true, + } + verify(200, app_body, deflate_or_gzip, options) do |status, headers, body| + headers.must_equal({ + 'Content-Encoding' => 'gzip', + 'Vary' => 'Accept-Encoding', + 'Content-Type' => 'text/plain' + }) + + buf = '' + raw_bytes = 0 + inflater = auto_inflater + body.each do |part| + raw_bytes += part.bytesize + buf << inflater.inflate(part) + end + buf << inflater.finish + expect = "hello\n" * 21 + buf.must_equal expect + raw_bytes.must_be(:<, expect.bytesize) + end + end end -- cgit v1.2.3-24-ge0c7 From be758b9c0311bd820be485949a5d5ea99dfabd0b Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Thu, 29 Jun 2017 01:59:51 +0000 Subject: deflater: additional mtime tests The next commit will reduce long-lived Time objects. Regardless of whether that commit is acceptable, having extra tests for existing mtime behavior cannot hurt. For testing responses with the Last-Modified header, setting a random date in the past should make failure to preserve mtime in the gzip header more apparent. --- test/spec_deflater.rb | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/test/spec_deflater.rb b/test/spec_deflater.rb index 410a1438..4a337cab 100644 --- a/test/spec_deflater.rb +++ b/test/spec_deflater.rb @@ -44,6 +44,8 @@ describe Rack::Deflater do [accept_encoding, accept_encoding.dup] end + start = Time.now.to_i + # build response status, headers, body = build_response( options['app_status'] || expected_status, @@ -67,6 +69,13 @@ describe Rack::Deflater do when 'gzip' io = StringIO.new(body_text) gz = Zlib::GzipReader.new(io) + mtime = gz.mtime.to_i + if last_mod = headers['Last-Modified'] + Time.httpdate(last_mod).to_i.must_equal mtime + else + mtime.must_be(:<=, Time.now.to_i) + mtime.must_be(:>=, start.to_i) + end tmp = gz.read gz.close tmp @@ -243,7 +252,7 @@ describe Rack::Deflater do end it 'handle gzip response with Last-Modified header' do - last_modified = Time.now.httpdate + last_modified = Time.at(123).httpdate options = { 'response_headers' => { 'Content-Type' => 'text/plain', -- cgit v1.2.3-24-ge0c7 From 69a2a195d749fdc2c04451688f3569bd5ce24c73 Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Thu, 29 Jun 2017 01:59:51 +0000 Subject: deflater: reduce live Time objects Only create a Time object if the Last-Modified header exists, (immediately use the Integer result of Time#to_). Otherwise, we can rely on the default behavior of Zlib::GzipWriter of setting the mtime to the current time. While we're at it, avoid repeating the hash lookup for Last-Modified. This results in an improvement from 11.0k to 11.4k iterations/sec with the following script: require 'benchmark/ips' require 'rack/mock' req = Rack::MockRequest.env_for('', 'HTTP_ACCEPT_ENCODING' => 'gzip') response = [200, {}, []] deflater = Rack::Deflater.new(lambda { |env| response }) Benchmark.ips do |x| x.report('deflater') do s, h, b = deflater.call(req) b.each { |buf| } b.close end end --- lib/rack/deflater.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/rack/deflater.rb b/lib/rack/deflater.rb index 821f708b..0d0d021f 100644 --- a/lib/rack/deflater.rb +++ b/lib/rack/deflater.rb @@ -58,8 +58,8 @@ module Rack when "gzip" headers['Content-Encoding'] = "gzip" headers.delete('Content-Length') - mtime = headers.key?("Last-Modified") ? - Time.httpdate(headers["Last-Modified"]) : Time.now + mtime = headers["Last-Modified"] + mtime = Time.httpdate(mtime).to_i if mtime [status, headers, GzipStream.new(body, mtime, @sync)] when "identity" [status, headers, body] @@ -80,7 +80,7 @@ module Rack def each(&block) @writer = block gzip =::Zlib::GzipWriter.new(self) - gzip.mtime = @mtime + gzip.mtime = @mtime if @mtime @body.each { |part| gzip.write(part) gzip.flush if @sync -- cgit v1.2.3-24-ge0c7