Permalink
Browse files

Merge pull request #3895 from kennyj/should_use_freezed_constant

Should use freezed string constant here.
  • Loading branch information...
2 parents 6867212 + 0a958d8 commit 12be0a59856d4f92a85c479843e296342be47789 @josevalim josevalim committed Dec 7, 2011
@@ -4,14 +4,18 @@ module ActionDispatch
module Http
module Cache
module Request
+
+ HTTP_IF_MODIFIED_SINCE = 'HTTP_IF_MODIFIED_SINCE'.freeze
+ HTTP_IF_NONE_MATCH = 'HTTP_IF_NONE_MATCH'.freeze
+
def if_modified_since
- if since = env['HTTP_IF_MODIFIED_SINCE']
+ if since = env[HTTP_IF_MODIFIED_SINCE]
Time.rfc2822(since) rescue nil
end
end
def if_none_match
- env['HTTP_IF_NONE_MATCH']
+ env[HTTP_IF_NONE_MATCH]
end
def not_modified?(modified_at)
@@ -43,31 +47,35 @@ module Response
alias :etag? :etag
def last_modified
- if last = headers['Last-Modified']
+ if last = headers[LAST_MODIFIED]
Time.httpdate(last)
end
end
def last_modified?
- headers.include?('Last-Modified')
+ headers.include?(LAST_MODIFIED)
end
def last_modified=(utc_time)
- headers['Last-Modified'] = utc_time.httpdate
+ headers[LAST_MODIFIED] = utc_time.httpdate
end
def etag=(etag)
key = ActiveSupport::Cache.expand_cache_key(etag)
- @etag = self["ETag"] = %("#{Digest::MD5.hexdigest(key)}")
+ @etag = self[ETAG] = %("#{Digest::MD5.hexdigest(key)}")
end
private
+ LAST_MODIFIED = "Last-Modified".freeze
+ ETAG = "ETag".freeze
+ CACHE_CONTROL = "Cache-Control".freeze
+
def prepare_cache_control!
@cache_control = {}
- @etag = self["ETag"]
+ @etag = self[ETAG]
- if cache_control = self["Cache-Control"]
+ if cache_control = self[CACHE_CONTROL]
cache_control.split(/,\s*/).each do |segment|
first, last = segment.split("=")
@cache_control[first.to_sym] = last || true
@@ -81,28 +89,32 @@ def handle_conditional_get!
end
end
- DEFAULT_CACHE_CONTROL = "max-age=0, private, must-revalidate"
+ DEFAULT_CACHE_CONTROL = "max-age=0, private, must-revalidate".freeze
+ NO_CACHE = "no-cache".freeze
+ PUBLIC = "public".freeze
+ PRIVATE = "private".freeze
+ MUST_REVALIDATE = "must-revalidate".freeze
def set_conditional_cache_control!
- return if self["Cache-Control"].present?
+ return if self[CACHE_CONTROL].present?
control = @cache_control
if control.empty?
- headers["Cache-Control"] = DEFAULT_CACHE_CONTROL
+ headers[CACHE_CONTROL] = DEFAULT_CACHE_CONTROL
elsif control[:no_cache]
- headers["Cache-Control"] = "no-cache"
+ headers[CACHE_CONTROL] = NO_CACHE
else
extras = control[:extras]
max_age = control[:max_age]
options = []
options << "max-age=#{max_age.to_i}" if max_age
- options << (control[:public] ? "public" : "private")
- options << "must-revalidate" if control[:must_revalidate]
+ options << (control[:public] ? PUBLIC : PRIVATE)
+ options << MUST_REVALIDATE if control[:must_revalidate]
options.concat(extras) if extras
- headers["Cache-Control"] = options.join(", ")
+ headers[CACHE_CONTROL] = options.join(", ")
end
end
end
@@ -20,6 +20,8 @@ def enabled?
@filters.present?
end
+ FILTERED = '[FILTERED]'.freeze
+
def compiled_filter
@compiled_filter ||= begin
regexps, blocks = compile_filter
@@ -29,7 +31,7 @@ def compiled_filter
original_params.each do |key, value|
if regexps.find { |r| key =~ r }
- value = '[FILTERED]'
+ value = FILTERED
elsif value.is_a?(Hash)
value = filter(value)
elsif value.is_a?(Array)
@@ -53,8 +53,10 @@ class Response
# information.
attr_accessor :charset, :content_type
- CONTENT_TYPE = "Content-Type"
-
+ CONTENT_TYPE = "Content-Type".freeze
+ SET_COOKIE = "Set-Cookie".freeze
+ LOCATION = "Location".freeze
+
cattr_accessor(:default_charset) { "utf-8" }
include Rack::Response::Helpers
@@ -66,7 +68,7 @@ def initialize(status = 200, header = {}, body = [])
@sending_file = false
@blank = false
- if content_type = self["Content-Type"]
+ if content_type = self[CONTENT_TYPE]
type, charset = content_type.split(/;\s*charset=/)
@content_type = Mime::Type.lookup(type)
@charset = charset || self.class.default_charset
@@ -142,12 +144,12 @@ def delete_cookie(key, value={})
end
def location
- headers['Location']
+ headers[LOCATION]
end
alias_method :redirect_url, :location
def location=(url)
- headers['Location'] = url
+ headers[LOCATION] = url
end
def close
@@ -158,10 +160,10 @@ def to_a
assign_default_content_type_and_charset!
handle_conditional_get!
- @header["Set-Cookie"] = @header["Set-Cookie"].join("\n") if @header["Set-Cookie"].respond_to?(:join)
+ @header[SET_COOKIE] = @header[SET_COOKIE].join("\n") if @header[SET_COOKIE].respond_to?(:join)
if [204, 304].include?(@status)
- @header.delete "Content-Type"
+ @header.delete CONTENT_TYPE
[@status, @header, []]
else
[@status, @header, self]
@@ -175,7 +177,7 @@ def to_a
# assert_equal 'AuthorOfNewPage', r.cookies['author']
def cookies
cookies = {}
- if header = self["Set-Cookie"]
+ if header = self[SET_COOKIE]
header = header.split("\n") if header.respond_to?(:to_str)
header.each do |cookie|
if pair = cookie.split(';').first

14 comments on commit 12be0a5

Contributor

gmile replied Dec 7, 2011

Why?

Member

sikachu replied Dec 7, 2011

Quoted from #3895:

We should use freezed string constant here.
Some string literals is used repeatedly.

So I think this is for memory optimization ;)

Contributor

gmile replied Dec 7, 2011

Interesting. Thank you!

Owner

jeremy replied Dec 7, 2011

Avoiding extra string object creation is nice, but freezing is unnecessary.

Member

sikachu replied Dec 7, 2011

Correct me if I'm wrong, but I think it is to make the string immutable, as we wouldn't want someone to play around with that constant right?

irb(main):001:0> FOO = 'foo'
=> "foo"
irb(main):002:0> FOO.sub!(/.+/, 'bar')
=> "bar"
irb(main):003:0> FOO
=> "bar"
irb(main):004:0> FOO.freeze
=> "bar"
irb(main):005:0> FOO.sub!(/.+/, 'baz')
TypeError: can't modify frozen string
    from (irb):5:in `sub!'
    from (irb):5
Owner

tenderlove replied Dec 7, 2011

Member

jonleighton replied Dec 7, 2011

+1 to not using freeze. Using constants to cache strings if fine by me, but freeze annoys me.

Contributor

josevalim replied Dec 7, 2011

Contributor

josevalim replied Dec 7, 2011

Member

jonleighton replied Dec 7, 2011

Heh, it only annoys me enough to bitch on github, not to actually change it ;)

Member

sikachu replied Dec 7, 2011

@tenderlove I surely learned that today. So yeah, +1 on not using freeze.

I've submitted a pull request #3897 :P

Owner

tenderlove replied Dec 7, 2011

@josevalim it consumes less memory when the strings are keys to a hash. If the keys are strings, hashes will dup them unless they are frozen.

require 'minitest/autorun'

class HashTest < MiniTest::Unit::TestCase
  def test_hash_key_dups
    x = "hi mom"
    z = { x => :y }
    refute_equal x.object_id, z.keys.first.object_id
  end

  ###
  # Freezing prevents hashes from duping strings when strings are keys.
  def test_frozen_does_not_dup
    x = "hi mom".freeze
    z = { x => :y }
    assert_equal x.object_id, z.keys.first.object_id
  end
end
Contributor

josevalim replied Dec 7, 2011

Contributor

kennyj replied Dec 8, 2011

Hey, guys !
Thanks for your many discussions. I was sleeping...

Sorry, a commit message was not appropriate.
My patch purpose was consist of two point.

1: less memory consumption (and less memory allocation time)
=> I thought that we should extract constant.

2: avoid someone accidentally changing it
=> I thought that we should freeze constants.

Please sign in to comment.