* [PATCH 1/5] deflater: rely on frozen_string_literal
2017-06-29 2:19 [PATCH 0/5] deflater: tiny optimizations + 1 new feature Eric Wong
@ 2017-06-29 2:19 ` Eric Wong
2017-06-29 2:19 ` [PATCH 2/5] deflater: avoid wasting an ivar slot on @closed Eric Wong
` (4 subsequent siblings)
5 siblings, 0 replies; 8+ messages in thread
From: Eric Wong @ 2017-06-29 2:19 UTC (permalink / raw)
To: rack-devel
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 46d5b20..9b798ba 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)
--
EW
--
---
You received this message because you are subscribed to the Google Groups "Rack Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rack-devel+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/5] deflater: avoid wasting an ivar slot on @closed
2017-06-29 2:19 [PATCH 0/5] deflater: tiny optimizations + 1 new feature Eric Wong
2017-06-29 2:19 ` [PATCH 1/5] deflater: rely on frozen_string_literal Eric Wong
@ 2017-06-29 2:19 ` Eric Wong
2017-06-29 2:19 ` [PATCH 3/5] deflater: support "sync: false" option Eric Wong
` (3 subsequent siblings)
5 siblings, 0 replies; 8+ messages in thread
From: Eric Wong @ 2017-06-29 2:19 UTC (permalink / raw)
To: rack-devel
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 9b798ba..d575adf 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
--
EW
--
---
You received this message because you are subscribed to the Google Groups "Rack Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rack-devel+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 3/5] deflater: support "sync: false" option
2017-06-29 2:19 [PATCH 0/5] deflater: tiny optimizations + 1 new feature Eric Wong
2017-06-29 2:19 ` [PATCH 1/5] deflater: rely on frozen_string_literal Eric Wong
2017-06-29 2:19 ` [PATCH 2/5] deflater: avoid wasting an ivar slot on @closed Eric Wong
@ 2017-06-29 2:19 ` Eric Wong
2017-06-29 2:19 ` [PATCH 4/5] deflater: additional mtime tests Eric Wong
` (2 subsequent siblings)
5 siblings, 0 replies; 8+ messages in thread
From: Eric Wong @ 2017-06-29 2:19 UTC (permalink / raw)
To: rack-devel
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 d575adf..821f708 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 0f27c85..410a143 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
--
EW
--
---
You received this message because you are subscribed to the Google Groups "Rack Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rack-devel+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 4/5] deflater: additional mtime tests
2017-06-29 2:19 [PATCH 0/5] deflater: tiny optimizations + 1 new feature Eric Wong
` (2 preceding siblings ...)
2017-06-29 2:19 ` [PATCH 3/5] deflater: support "sync: false" option Eric Wong
@ 2017-06-29 2:19 ` Eric Wong
2017-06-29 2:19 ` [PATCH 5/5] deflater: reduce live Time objects Eric Wong
2017-07-24 1:33 ` [PATCH 0/5] deflater: tiny optimizations + 1 new feature Eric Wong
5 siblings, 0 replies; 8+ messages in thread
From: Eric Wong @ 2017-06-29 2:19 UTC (permalink / raw)
To: rack-devel
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 410a143..4a337ca 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',
--
EW
--
---
You received this message because you are subscribed to the Google Groups "Rack Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rack-devel+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 5/5] deflater: reduce live Time objects
2017-06-29 2:19 [PATCH 0/5] deflater: tiny optimizations + 1 new feature Eric Wong
` (3 preceding siblings ...)
2017-06-29 2:19 ` [PATCH 4/5] deflater: additional mtime tests Eric Wong
@ 2017-06-29 2:19 ` Eric Wong
2017-07-24 1:33 ` [PATCH 0/5] deflater: tiny optimizations + 1 new feature Eric Wong
5 siblings, 0 replies; 8+ messages in thread
From: Eric Wong @ 2017-06-29 2:19 UTC (permalink / raw)
To: rack-devel
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 821f708..0d0d021 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
--
EW
--
---
You received this message because you are subscribed to the Google Groups "Rack Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rack-devel+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 0/5] deflater: tiny optimizations + 1 new feature
2017-06-29 2:19 [PATCH 0/5] deflater: tiny optimizations + 1 new feature Eric Wong
` (4 preceding siblings ...)
2017-06-29 2:19 ` [PATCH 5/5] deflater: reduce live Time objects Eric Wong
@ 2017-07-24 1:33 ` Eric Wong
2017-07-25 1:11 ` Rafael Mendonça França
5 siblings, 1 reply; 8+ messages in thread
From: Eric Wong @ 2017-07-24 1:33 UTC (permalink / raw)
To: rack-devel
Ping on this series, I might be working on a Rack app which will
depend on "sync: false" for best results
* [PATCH 0/5] deflater: tiny optimizations + 1 new feature
https://public-inbox.org/rack-devel/20170629021915.22517-1-e@80x24.org/
And also a minor common_logger tweak:
* [PATCH] common_logger: rely on monotonic clock
https://public-inbox.org/rack-devel/20170629023056.GA22961@untitled/
Thanks.
--
---
You received this message because you are subscribed to the Google Groups "Rack Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rack-devel+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
^ permalink raw reply [flat|nested] 8+ messages in thread