From eb9d72aea164a050f06e39bddd66150cd86f6a4d Mon Sep 17 00:00:00 2001 From: Peter Zhu Date: Sat, 25 Jul 2026 20:14:58 -0400 Subject: [PATCH] Max length for embedding duplicated strings 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. --- string.c | 33 +++++++++++++--------------- test/-ext-/string/test_rb_str_dup.rb | 6 +++-- test/objspace/test_objspace.rb | 4 +++- 3 files changed, 22 insertions(+), 21 deletions(-) diff --git a/string.c b/string.c index 5339b9ac0b5d15..b54051f1a6f000 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 c76a90252f7040..13f07e290613f8 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-/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 403e98e6e1608d..c96222e7630c3e 100644 --- a/test/objspace/test_objspace.rb +++ b/test/objspace/test_objspace.rb @@ -28,8 +28,10 @@ def test_memsize_of 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?}