Detect in-place modifications on Strings #15788

Merged
merged 1 commit into from Jun 27, 2014

Conversation

Projects
None yet
2 participants
@sgrif
Member

sgrif commented Jun 18, 2014

No description provided.

@@ -41,6 +41,7 @@ def reload(*)
def initialize_dup(other) # :nodoc:
super
calculate_changes_from_defaults
+ @original_raw_attributes = original_raw_attributes.dup

This comment has been minimized.

@rafaelfranca

rafaelfranca Jun 18, 2014

Member

Just to understand, why we are needing to dup this now?

@rafaelfranca

rafaelfranca Jun 18, 2014

Member

Just to understand, why we are needing to dup this now?

This comment has been minimized.

@sgrif

sgrif Jun 18, 2014

Member

Turns out we don't. There was a different issue causing this.

@sgrif

sgrif Jun 18, 2014

Member

Turns out we don't. There was a different issue causing this.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Jun 27, 2014

Member

@sgrif does the object allocation grew substantially with this change? For what I saw we are allocating a new string for each string attribute every time we are going to write the model instance right?

Member

rafaelfranca commented Jun 27, 2014

@sgrif does the object allocation grew substantially with this change? For what I saw we are allocating a new string for each string attribute every time we are going to write the model instance right?

@sgrif

This comment has been minimized.

Show comment
Hide comment
@sgrif

sgrif Jun 27, 2014

Member

Correct, this will increase string allocations, however in context of the
rest of the work done, the performance penalty is extremely small.
On Jun 27, 2014 12:09 PM, "Rafael Mendonça França" notifications@github.com
wrote:

@sgrif https://github.com/sgrif does the object allocation grew
substantially with this change? For what I saw we are allocating a new
string for each string attribute every time we are going to write the model
instance right?


Reply to this email directly or view it on GitHub
#15788 (comment).

Member

sgrif commented Jun 27, 2014

Correct, this will increase string allocations, however in context of the
rest of the work done, the performance penalty is extremely small.
On Jun 27, 2014 12:09 PM, "Rafael Mendonça França" notifications@github.com
wrote:

@sgrif https://github.com/sgrif does the object allocation grew
substantially with this change? For what I saw we are allocating a new
string for each string attribute every time we are going to write the model
instance right?


Reply to this email directly or view it on GitHub
#15788 (comment).

@rafaelfranca rafaelfranca merged commit c7802dc into rails:master Jun 27, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

rafaelfranca added a commit that referenced this pull request Jun 27, 2014

Merge pull request #15788 from sgrif/sg-mutable-strings
Detect in-place modifications on Strings

sgrif added a commit that referenced this pull request Oct 15, 2015

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.

@sgrif sgrif deleted the sgrif:sg-mutable-strings branch Mar 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment