diff options
author | Sean Griffin <sean@seantheprogrammer.com> | 2015-10-15 09:40:21 -0700 |
---|---|---|
committer | Sean Griffin <sean@seantheprogrammer.com> | 2015-10-15 09:50:37 -0700 |
commit | 34321e4a433bb7eef48fd743286601403f8f7d82 (patch) | |
tree | 45ffbd21ae35cb5002566a4327e5addb9d005d47 /activemodel | |
parent | de92b06bca9e744525452dd83e13a784bcfb9d3e (diff) | |
download | rails-34321e4a433bb7eef48fd743286601403f8f7d82.tar.gz rails-34321e4a433bb7eef48fd743286601403f8f7d82.tar.bz2 rails-34321e4a433bb7eef48fd743286601403f8f7d82.zip |
Add an immutable string type to opt out of string duping
This type adds an escape hatch to apps for which string duping causes
unacceptable memory growth. The reason we are duping them is in order to
detect mutation, which was a feature added to 4.2 in #15674. The string
type was modified to support this behavior in #15788.
Memory growth is really only a concern for string types, as it's the
only mutable type where the act of coersion does not create a new object
regardless (as we're usually returning an object of a different class).
I do feel strongly that if we are going to support detecting mutation,
we should do it universally for any type which is mutable. While it is
less common and ideomatic to mutate strings than arrays or hashes, there
shouldn't be rules or gotchas to understanding our behavior.
However, I also appreciate that for apps which are using a lot of string
columns, this would increase the number of allocations by a large
factor. To ensure that we keep our contract, if you'd like to opt out of
mutation detection on strings, you'll also be option out of mutation of
those strings.
I'm not completely married to the thought that strings coming out of
this actually need to be frozen -- and I think the name is correct
either way, as the purpose of this is to provide a string type which
does not detect mutation.
In the new implementation, I'm only overriding `cast_value`. I did not
port over the duping in `serialize`. I cannot think of a reason we'd
need to dup the string there, and the tests pass without it.
Unfortunately that line was introduced at a time where I was not nearly
as good about writing my commit messages, so I have no context as to
why I added it. Thanks past Sean. You are a jerk.
Diffstat (limited to 'activemodel')
-rw-r--r-- | activemodel/lib/active_model/type.rb | 2 | ||||
-rw-r--r-- | activemodel/lib/active_model/type/immutable_string.rb | 28 | ||||
-rw-r--r-- | activemodel/lib/active_model/type/string.rb | 28 | ||||
-rw-r--r-- | activemodel/test/cases/type/string_test.rb | 7 |
4 files changed, 45 insertions, 20 deletions
diff --git a/activemodel/lib/active_model/type.rb b/activemodel/lib/active_model/type.rb index f8ca7d0512..bec851594f 100644 --- a/activemodel/lib/active_model/type.rb +++ b/activemodel/lib/active_model/type.rb @@ -9,6 +9,7 @@ require 'active_model/type/date_time' require 'active_model/type/decimal' require 'active_model/type/decimal_without_scale' require 'active_model/type/float' +require 'active_model/type/immutable_string' require 'active_model/type/integer' require 'active_model/type/string' require 'active_model/type/text' @@ -49,6 +50,7 @@ module ActiveModel register(:date_time, Type::DateTime) register(:decimal, Type::Decimal) register(:float, Type::Float) + register(:immutable_string, Type::ImmutableString) register(:integer, Type::Integer) register(:string, Type::String) register(:text, Type::Text) diff --git a/activemodel/lib/active_model/type/immutable_string.rb b/activemodel/lib/active_model/type/immutable_string.rb new file mode 100644 index 0000000000..cfa4b3fb4b --- /dev/null +++ b/activemodel/lib/active_model/type/immutable_string.rb @@ -0,0 +1,28 @@ +module ActiveModel + module Type + class ImmutableString < Value # :nodoc: + def type + :string + end + + def serialize(value) + case value + when ::Numeric, ActiveSupport::Duration then value.to_s + when true then "t" + when false then "f" + else super + end + end + + private + + def cast_value(value) + case value + when true then "t" + when false then "f" + else value.to_s.freeze + end + end + end + end +end diff --git a/activemodel/lib/active_model/type/string.rb b/activemodel/lib/active_model/type/string.rb index fd1630c751..4c8dc15778 100644 --- a/activemodel/lib/active_model/type/string.rb +++ b/activemodel/lib/active_model/type/string.rb @@ -1,34 +1,22 @@ +require "active_model/type/immutable_string" + module ActiveModel module Type - class String < Value # :nodoc: - def type - :string - end - + class String < ImmutableString # :nodoc: def changed_in_place?(raw_old_value, new_value) if new_value.is_a?(::String) raw_old_value != new_value end end - def serialize(value) - case value - when ::Numeric, ActiveSupport::Duration then value.to_s - when ::String then ::String.new(value) - when true then "t" - when false then "f" - else super - end - end - private def cast_value(value) - case value - when true then "t" - when false then "f" - # String.new is slightly faster than dup - else ::String.new(value.to_s) + result = super + if ::String === result + ::String.new(result) + else + result end end end diff --git a/activemodel/test/cases/type/string_test.rb b/activemodel/test/cases/type/string_test.rb index 8ec771ea42..7b25a1ef74 100644 --- a/activemodel/test/cases/type/string_test.rb +++ b/activemodel/test/cases/type/string_test.rb @@ -10,6 +10,13 @@ module ActiveModel assert_equal "123", type.cast(123) end + test "immutable strings are not duped coming out" do + s = "foo" + type = Type::ImmutableString.new + assert_same s, type.cast(s) + assert_same s, type.deserialize(s) + end + test "values are duped coming out" do s = "foo" type = Type::String.new |