From 0adbeeb0c92c6de2e4a148e4b54d56cd4a325800 Mon Sep 17 00:00:00 2001 From: Yehuda Katz Date: Mon, 10 Aug 2009 11:40:41 -0400 Subject: [PATCH] Got overhead down from 127 to 85. All tests pass: * Tentatively replaced HeaderHash with SimpleHeaderHash, which does not preserve case but does handle converting Arrays to Strings in to_hash. This requires further discussion. * Moved default_charset to ActionDispatch::Response to avoid having to hop over to ActionController. Ideally, this would be a constant on AD::Response, but some tests expect to be able to change it dynamically and I didn't want to change them yet. * Completely override #initialize from Rack::Response. Previously, it was creating a HeaderHash, and then we were creating an entirely new one. There is no way to call super without incurring the overhead of creating a HeaderHash. * Override #write from Rack::Response. Its implementation tracks Content-Length, and doing so adds additional overhead that could be mooted if other middleware changes the body. It is more efficiently done at the top-level server. * Change sending_file to an instance_variable instead of header inspection. In general, if a state is important, it should be set as a property of the response not reconstructed later. * Set the Etag to @body instead of .body. AS::Cache.expand_cache_key handles Arrays fine, and it's more efficient to let it handle the body parts, since it is not forced to create a joined String. * If we detect the default cache control case, just set it, rather than setting the constituent parts and then running the normal (expensive) code to generate the string. --- .../action_controller/metal/compatibility.rb | 5 +- .../lib/action_controller/metal/streaming.rb | 3 +- .../lib/action_controller/testing/process.rb | 2 +- .../lib/action_dispatch/http/response.rb | 94 +++++++++++-------- .../core_ext/class/attribute_accessors.rb | 5 +- 5 files changed, 66 insertions(+), 43 deletions(-) diff --git a/actionpack/lib/action_controller/metal/compatibility.rb b/actionpack/lib/action_controller/metal/compatibility.rb index f94d1c669c..a008b53d45 100644 --- a/actionpack/lib/action_controller/metal/compatibility.rb +++ b/actionpack/lib/action_controller/metal/compatibility.rb @@ -25,8 +25,9 @@ module ActionController cattr_accessor :relative_url_root self.relative_url_root = ENV['RAILS_RELATIVE_URL_ROOT'] - cattr_accessor :default_charset - self.default_charset = "utf-8" + class << self + delegate :default_charset=, :to => "ActionDispatch::Response" + end # cattr_reader :protected_instance_variables cattr_accessor :protected_instance_variables diff --git a/actionpack/lib/action_controller/metal/streaming.rb b/actionpack/lib/action_controller/metal/streaming.rb index 57318e8747..4761763a26 100644 --- a/actionpack/lib/action_controller/metal/streaming.rb +++ b/actionpack/lib/action_controller/metal/streaming.rb @@ -145,7 +145,6 @@ module ActionController #:nodoc: def send_data(data, options = {}) #:doc: logger.info "Sending data #{options[:filename]}" if logger send_file_headers! options.merge(:length => data.bytesize) - @performed_render = false render :status => options[:status], :text => data end @@ -175,6 +174,8 @@ module ActionController #:nodoc: 'Content-Transfer-Encoding' => 'binary' ) + response.sending_file = true + # Fix a problem with IE 6.0 on opening downloaded files: # If Cache-Control: no-cache is set (which Rails does by default), # IE removes the file it just downloaded from its cache immediately diff --git a/actionpack/lib/action_controller/testing/process.rb b/actionpack/lib/action_controller/testing/process.rb index d32d5562e8..147a7e7631 100644 --- a/actionpack/lib/action_controller/testing/process.rb +++ b/actionpack/lib/action_controller/testing/process.rb @@ -52,7 +52,7 @@ module ActionController #:nodoc: class TestResponse < ActionDispatch::TestResponse def recycle! @status = 200 - @header = Rack::Utils::HeaderHash.new + @header = ActionDispatch::Response::SimpleHeaderHash.new @writer = lambda { |x| @body << x } @block = nil @length = 0 diff --git a/actionpack/lib/action_dispatch/http/response.rb b/actionpack/lib/action_dispatch/http/response.rb index 03d1780b77..1d3cf63984 100644 --- a/actionpack/lib/action_dispatch/http/response.rb +++ b/actionpack/lib/action_dispatch/http/response.rb @@ -32,18 +32,42 @@ module ActionDispatch # :nodoc: # end # end class Response < Rack::Response + class SimpleHeaderHash < Hash + def to_hash + result = {} + each do |k,v| + v = v.join("\n") if v.is_a?(Array) + result[k] = v + end + result + end + end + attr_accessor :request attr_reader :cache_control - attr_writer :header + attr_writer :header, :sending_file alias_method :headers=, :header= - delegate :default_charset, :to => 'ActionController::Base' - def initialize - super + @status = 200 + @header = SimpleHeaderHash.new @cache_control = {} - @header = Rack::Utils::HeaderHash.new + + @writer = lambda { |x| @body << x } + @block = nil + @length = 0 + + @body = [] + @sending_file = false + + yield self if block_given? + end + + def write(str) + s = str.to_s + @writer.call s + str end def status=(status) @@ -128,20 +152,20 @@ module ActionDispatch # :nodoc: end end - def sending_file? - headers["Content-Transfer-Encoding"] == "binary" - end + CONTENT_TYPE = "Content-Type" + + cattr_accessor(:default_charset) { "utf-8" } def assign_default_content_type_and_charset! - return if !headers["Content-Type"].blank? + return if !headers[CONTENT_TYPE].blank? @content_type ||= Mime::HTML - @charset ||= default_charset + @charset ||= self.class.default_charset type = @content_type.to_s.dup - type << "; charset=#{@charset}" unless sending_file? + type << "; charset=#{@charset}" unless @sending_file - headers["Content-Type"] = type + headers[CONTENT_TYPE] = type end def prepare! @@ -168,17 +192,6 @@ module ActionDispatch # :nodoc: str end - def set_cookie(key, value) - if value.has_key?(:http_only) - ActiveSupport::Deprecation.warn( - "The :http_only option in ActionController::Response#set_cookie " + - "has been renamed. Please use :httponly instead.", caller) - value[:httponly] ||= value.delete(:http_only) - end - - super(key, value) - end - # Returns the response cookies, converted to a Hash of (name => value) pairs # # assert_equal 'AuthorOfNewPage', r.cookies['author'] @@ -201,7 +214,7 @@ module ActionDispatch # :nodoc: if etag? || last_modified? || !cache_control.empty? set_conditional_cache_control! elsif nonempty_ok_response? - self.etag = body + self.etag = @body if request && request.etag_matches?(etag) self.status = 304 @@ -214,30 +227,37 @@ module ActionDispatch # :nodoc: end end + EMPTY_RESPONSE = [" "] + def nonempty_ok_response? ok = !@status || @status == 200 - ok && string_body? + ok && string_body? && @body != EMPTY_RESPONSE end def string_body? !body_parts.respond_to?(:call) && body_parts.any? && body_parts.all? { |part| part.is_a?(String) } end + DEFAULT_CACHE_CONTROL = "max-age=0, private, must-revalidate" + def set_conditional_cache_control! - if cache_control.empty? - cache_control.merge!(:public => false, :max_age => 0, :must_revalidate => true) + control = cache_control + + if control.empty? + headers["Cache-Control"] = DEFAULT_CACHE_CONTROL + else + extras = control[:extras] + max_age = control[:max_age] + + options = [] + options << "max-age=#{max_age}" if max_age + options << (control[:public] ? "public" : "private") + options << "must-revalidate" if control[:must_revalidate] + options.concat(extras) if extras + + headers["Cache-Control"] = options.join(", ") end - public_cache, max_age, must_revalidate, extras = - cache_control.values_at(:public, :max_age, :must_revalidate, :extras) - - options = [] - options << "max-age=#{max_age}" if max_age - options << (public_cache ? "public" : "private") - options << "must-revalidate" if must_revalidate - options.concat(extras) if extras - - headers["Cache-Control"] = options.join(", ") end end end diff --git a/activesupport/lib/active_support/core_ext/class/attribute_accessors.rb b/activesupport/lib/active_support/core_ext/class/attribute_accessors.rb index 74ce85a1c2..1602a609eb 100644 --- a/activesupport/lib/active_support/core_ext/class/attribute_accessors.rb +++ b/activesupport/lib/active_support/core_ext/class/attribute_accessors.rb @@ -46,11 +46,12 @@ class Class end # end " unless options[:instance_writer] == false } # # instance writer above is generated unless options[:instance_writer] == false EOS + self.send("#{sym}=", yield) if block_given? end end - def cattr_accessor(*syms) + def cattr_accessor(*syms, &blk) cattr_reader(*syms) - cattr_writer(*syms) + cattr_writer(*syms, &blk) end end