Max length for embedding duplicated stringsHEADmaster

We should cap the max length for embedding duplicated strings because it
could be costly in performance and memory usage for very large embedded
strings. Instead, we can use a view into the parent string and perform
copy-on-write.

3 files changed, 22 insertions, 21 deletions

diff --git a/string.c b/string.c
index 5339b9ac0b..b54051f1a6 100644
--- a/string.c
+++ b/string.c
@@ -1916,7 +1916,7 @@ ec_str_alloc_heap(struct rb_execution_context_struct *ec, VALUE klass)
     return (VALUE)str;
 }
 
-static inline VALUE
+static inline void
 str_duplicate_setup_encoding(VALUE str, VALUE dup, VALUE flags)
 {
     int encidx = 0;
@@ -1926,12 +1926,11 @@ str_duplicate_setup_encoding(VALUE str, VALUE dup, VALUE flags)
     }
     FL_SET_RAW(dup, flags & ~FL_FREEZE);
     if (encidx) rb_enc_associate_index(dup, encidx);
-    return dup;
 }
 
 static const VALUE flag_mask = ENC_CODERANGE_MASK | ENCODING_MASK | FL_FREEZE;
 
-static inline VALUE
+static inline void
 str_duplicate_setup_embed(VALUE klass, VALUE str, VALUE dup)
 {
     VALUE flags = FL_TEST_RAW(str, flag_mask);
@@ -1941,10 +1940,10 @@ str_duplicate_setup_embed(VALUE klass, VALUE str, VALUE dup)
     RUBY_ASSERT(str_embed_capa(dup) >= len + TERM_LEN(str));
     MEMCPY(RSTRING(dup)->as.embed.ary, RSTRING(str)->as.embed.ary, char, len + TERM_LEN(str));
     STR_SET_LEN(dup, RSTRING_LEN(str));
-    return str_duplicate_setup_encoding(str, dup, flags);
+    str_duplicate_setup_encoding(str, dup, flags);
 }
 
-static inline VALUE
+static inline void
 str_duplicate_setup_heap(VALUE klass, VALUE str, VALUE dup)
 {
     VALUE flags = FL_TEST_RAW(str, flag_mask);
@@ -1965,32 +1964,30 @@ str_duplicate_setup_heap(VALUE klass, VALUE str, VALUE dup)
     flags |= RSTRING_NOEMBED | STR_SHARED;
 
     STR_SET_LEN(dup, RSTRING_LEN(str));
-    return str_duplicate_setup_encoding(str, dup, flags);
+    str_duplicate_setup_encoding(str, dup, flags);
 }
 
-static inline VALUE
-str_duplicate_setup(VALUE klass, VALUE str, VALUE dup)
-{
-    if (STR_EMBED_P(str)) {
-        return str_duplicate_setup_embed(klass, str, dup);
-    }
-    else {
-        return str_duplicate_setup_heap(klass, str, dup);
-    }
-}
+/* Force duplicated strings above 1024 bytes to be views rather than copies since
+ * copying will use memory and have significant overhead.
+ * Calculated as: 1024 - header size - NUL terminator size */
+#define STR_DUPLICATE_MAX_EMBED_LEN ((long)(1024 - offsetof(struct RString, as.embed) - 1))
 
 static inline VALUE
 str_duplicate(VALUE klass, VALUE str)
 {
     VALUE dup;
-    if (STR_EMBED_P(str)) {
+    if (STR_EMBED_P(str) && RSTRING_LEN(str) <= STR_DUPLICATE_MAX_EMBED_LEN) {
         dup = str_alloc_embed(klass, RSTRING_LEN(str) + TERM_LEN(str));
+
+        str_duplicate_setup_embed(klass, str, dup);
     }
     else {
         dup = str_alloc_heap(klass);
+
+        str_duplicate_setup_heap(klass, str, dup);
     }
 
-    return str_duplicate_setup(klass, str, dup);
+    return dup;
 }
 
 VALUE
diff --git a/test/-ext-/string/test_rb_str_dup.rb b/test/-ext-/string/test_rb_str_dup.rb
index c76a90252f..13f07e2906 100644
--- a/test/-ext-/string/test_rb_str_dup.rb
+++ b/test/-ext-/string/test_rb_str_dup.rb
@@ -2,15 +2,17 @@ require 'test/unit'
 require '-test-/string'
 
 class Test_RbStrDup < Test::Unit::TestCase
+  STR_DUPLICATE_MAX_EMBED_LEN = 999 # From macro defined in string.c
+
   def test_nested_shared_non_frozen
-    orig_str = "a" * GC::INTERNAL_CONSTANTS[:RVARGC_MAX_ALLOCATE_SIZE]
+    orig_str = "a" * (STR_DUPLICATE_MAX_EMBED_LEN + 1)
     str = Bug::String.rb_str_dup(Bug::String.rb_str_dup(orig_str))
     assert_send([Bug::String, :shared_string?, str])
     assert_not_send([Bug::String, :sharing_with_shared?, str], '[Bug #15792]')
   end
 
   def test_nested_shared_frozen
-    orig_str = "a" * GC::INTERNAL_CONSTANTS[:RVARGC_MAX_ALLOCATE_SIZE]
+    orig_str = "a" * (STR_DUPLICATE_MAX_EMBED_LEN + 1)
     str = Bug::String.rb_str_dup(Bug::String.rb_str_dup(orig_str).freeze)
     assert_send([Bug::String, :shared_string?, str])
     assert_not_send([Bug::String, :sharing_with_shared?, str], '[Bug #15792]')
diff --git a/test/objspace/test_objspace.rb b/test/objspace/test_objspace.rb
index 403e98e6e1..c96222e763 100644
--- a/test/objspace/test_objspace.rb
+++ b/test/objspace/test_objspace.rb
@@ -28,8 +28,10 @@ class TestObjSpace < Test::Unit::TestCase
                     ObjectSpace.memsize_of(//.match("")))
   end
 
+  STR_DUPLICATE_MAX_EMBED_LEN = 999 # From macro defined in string.c
+
   def test_memsize_of_root_shared_string
-    a = "a" * GC::INTERNAL_CONSTANTS[:RVARGC_MAX_ALLOCATE_SIZE]
+    a = "a" * (STR_DUPLICATE_MAX_EMBED_LEN + 1)
     b = a.dup
     c = nil
     ObjectSpace.each_object(String) {|x| break c = x if a == x and x.frozen?}