Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't call modify() on RubyStrings so eagerly when freezing them #3022

Merged
merged 1 commit into from Jun 5, 2015

Conversation

Projects
None yet
2 participants
@bbrowning
Copy link
Contributor

bbrowning commented Jun 5, 2015

This fixes #3019, where in certain cases it's possible to end up with
RubyString instances backed by very large unique ByteLists where only
a tiny portion of the bytes in the ByteList are actually needed.

What was happening is modify was being called on RubyString
instances too eagerly, resulting in unnecessary duplication of their
underlying ByteList instances instead of sharing when possible.

The two changes here are in RubyString#newFrozen and
RubyString#resize and I believe are more correct based on the C
implementation of these methods.

For RubyString#newFrozen, which roughly corresponds to
rb_str_new_frozen in
C (https://github.com/ruby/ruby/blob/v2_2_2/string.c#L932), the logic
looks to me to return the shared string and I don't see it calling
str_make_independent, which is roughly the same thing as
RubyString#modify. So, I just removed the call to modify.

For RubyString#resize, which roughly corresponds to to
rb_str_resize in C, shared strings are only made independent if
their length
differ (https://github.com/ruby/ruby/blob/v2_2_2/string.c#L2155). Thus,
instead of unconditionally calling modify here, I moved it into the
conditions that are true when the length differs.

Don't call modify() on RubyStrings so eagerly when freezing them
This fixes #3019, where in certain cases it's possible to end up with
RubyString instances backed by very large unique ByteLists where only
a tiny portion of the bytes in the ByteList are actually needed.

What was happening is `modify` was being called on RubyString
instances too eagerly, resulting in unnecessary duplication of their
underlying ByteList instances instead of sharing when possible.

The two changes here are in `RubyString#newFrozen` and
`RubyString#resize` and I believe are more correct based on the C
implementation of these methods.

For `RubyString#newFrozen`, which roughly corresponds to
`rb_str_new_frozen` in
C (https://github.com/ruby/ruby/blob/v2_2_2/string.c#L932), the logic
looks to me to return the shared string and I don't see it calling
`str_make_independent`, which is roughly the same thing as
`RubyString#modify`. So, I just removed the call to `modify`.

For `RubyString#resize`, which roughly corresponds to to
`rb_str_resize` in C, shared strings are only made independent if
their length
differ (https://github.com/ruby/ruby/blob/v2_2_2/string.c#L2155). Thus,
instead of unconditionally calling `modify` here, I moved it into the
conditions that are true when the length differs.

enebo added a commit that referenced this pull request Jun 5, 2015

Merge pull request #3022 from bbrowning/string-cow-oom
Don't call modify() on RubyStrings so eagerly when freezing them

@enebo enebo merged commit f4ede91 into jruby:master Jun 5, 2015

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@enebo enebo added this to the JRuby 9.0.0.0.rc1 milestone Jun 5, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.