string.c: keep what the bytes read as across String#reverse! - #7224

mrb_str_reverse_bang() opens by asking for the character count, and that
question walks the string:

/* src/string.c */
#ifdef MRB_UTF8_STRING
  mrb_int utf8_len = mrb_str_char_len(mrb, str);   /* walks, and records 7BIT */
  mrb_int len = RSTR_LEN(s);

  if (utf8_len < 2) return str;
  if (utf8_len < len) {
    mrb_str_modify(mrb, s);                        /* sets the record to UNKNOWN */
    ...
  }
#endif

  if (RSTR_LEN(s) > 1) {
    mrb_str_modify(mrb, s);                        /* same, on the single-byte path */
    goto bytes;
  }

mrb_str_char_len() scans with search_nonascii() and records 7BIT on the
string where the scan meets nothing else. mrb_str_modify() sets that record
back to UNKNOWN one line later, because a write in general can turn a sound
string unsound. So the walk is paid for, thrown away, and paid for again at the
next question about the string. In a loop that is once per reverse!, either
inside reverse! on the next iteration or in whatever asks first.

The change

Both calls become str_modify_keep_cr(), the in-file helper for a write that
leaves what the bytes read as standing:

/* src/string.c */
static void
str_modify_keep_cr(mrb_state *mrb, struct RString *s)
{
  mrb_check_frozen(mrb, s);
  str_unshare_buffer(mrb, s);
  if (RSTR_CODERANGE(s) == MRB_STR_CODERANGE_BROKEN) {
    RSTR_CODERANGE_SET(s, MRB_STR_CODERANGE_UNKNOWN);
  }
}

Reversing is such a write. The multi-byte path turns each character's bytes
around where they stand and then turns the whole buffer around, which puts the
characters back in the reverse order with each one whole; the single-byte path
is a byte reversal of a string holding one character per byte. Neither can turn
a sound string unsound, so the record the walk arrived at is still the answer,
and the helper asks a string already read as broken again on its own.

Performance

Instructions per call, callgrind on bin/mruby from the bintest build, as
Ir(2N) - Ir(N) so the startup cancels:

The ASCII string is where the walk was the cost. A string holding characters
above ASCII is counted by walking it either way, since the count is not kept on
the string, so keeping the record buys it nothing and costs it nothing.

Size

.text summed over every .o, each side built from an empty build directory:

src/string.o is the only object that moves in any of the six, and
mrb_str_reverse_bang() itself is 368 bytes on both sides of every one of
them. What moves is where gcc puts the two helpers, since the change takes two
callers off mrb_str_modify() and puts them on str_modify_keep_cr():

In bintest, mrb_str_modify() is down to a caller count gcc will inline it
at, so mrb_str_setbyte() now calls str_unshare_buffer() directly and carries
the rest of the body itself. In ascii-case it goes the other way: eight
callers is past where gcc keeps inlining str_modify_keep_cr(), so it is
emitted once and called, and the callers it had been inlined into give back
more than the 926 bytes that costs.

A build whose strings index by byte compiles RSTR_CODERANGE_SET() to nothing,
which makes the two helpers the same code, and byte-string is unchanged to
the byte.

Testing

build_config/ci/gcc-clang.rb and build_config/asan.rb, run per build so the
counts are attributable:

One more test than master everywhere the string is indexed by character;
byte-string drops mruby-encoding and with it the file the test lives in.
The binary tests pass 117 of 117 under ci/gcc-clang and 79 of 79 under
asan.

Beside the suite, 100,000 random strings built from ASCII, characters above
it, and stray bytes were reversed under both builds, comparing the bytes, the
length and valid_encoding? of the result and of the result reversed again.
Every line is identical.

The test that comes with this is for the record the change keeps. What a
reversed string answers for its own length and validity was asserted nowhere:
the existing reverse! tests read the result as a string and stop there, so a
reversal that kept a record it had unmade would have passed them. Dropping the
per-character reversal from the multi-byte path, which is what would leave such
a record wrong, turns the new assertions red.

Environment

# ci/gcc-clang, full-debug
gcc -std=gnu99 -g -O3 -Wall -Wundef -Werror-implicit-function-declaration -Wwrite-strings -g3 -O0 -DMRB_GC_STRESS -DMRB_USE_DEBUG_HOOK -DMRB_DEBUG -DMRB_USE_BIGINT -DMRB_USE_COMPLEX -DHAVE_MRUBY_ENCODING_GEM -DMRB_UTF8_STRING -DHAVE_MRUBY_IO_GEM -DMRB_USE_RATIONAL -DHAVE_MRUBY_REGEXP_GEM -DMRB_USE_SET -DMRB_USE_TASK_SCHEDULER src/string.c

# ci/gcc-clang, bintest
gcc -std=gnu99 -g -O3 -Wall -Wundef -Werror-implicit-function-declaration -Wwrite-strings -DMRB_GC_FIXED_ARENA -DMRB_USE_BIGINT -DMRB_USE_COMPLEX -DHAVE_MRUBY_ENCODING_GEM -DMRB_UTF8_STRING -DHAVE_MRUBY_IO_GEM -DMRB_USE_RATIONAL -DHAVE_MRUBY_REGEXP_GEM -DMRB_USE_SET -DMRB_USE_TASK_SCHEDULER -DMRB_USE_DEBUG_HOOK src/string.c

# ci/gcc-clang, cxx_abi
gcc -g -O3 -Wall -Wundef -Wwrite-strings -x c++ -std=gnu++03 -DMRB_GC_FIXED_ARENA -DMRB_USE_CXX_EXCEPTION -DMRB_USE_CXX_ABI -DMRB_USE_BIGINT -DMRB_USE_COMPLEX -DHAVE_MRUBY_ENCODING_GEM -DMRB_UTF8_STRING -DHAVE_MRUBY_IO_GEM -DMRB_USE_RATIONAL -DHAVE_MRUBY_REGEXP_GEM -DMRB_USE_SET -DMRB_USE_TASK_SCHEDULER src/string.c

# ci/gcc-clang, byte-string
gcc -std=gnu99 -g -O3 -Wall -Wundef -Werror-implicit-function-declaration -Wwrite-strings -DMRB_USE_BIGINT -DMRB_USE_COMPLEX -DHAVE_MRUBY_IO_GEM -DMRB_USE_RATIONAL -DHAVE_MRUBY_REGEXP_GEM -DMRB_USE_SET -DMRB_USE_TASK_SCHEDULER src/string.c

# ci/gcc-clang, ascii-case
gcc -std=gnu99 -g -O3 -Wall -Wundef -Werror-implicit-function-declaration -Wwrite-strings -DMRB_USE_ASCII_CASE -DMRB_USE_BIGINT -DMRB_USE_COMPLEX -DHAVE_MRUBY_ENCODING_GEM -DMRB_UTF8_STRING -DHAVE_MRUBY_IO_GEM -DMRB_USE_RATIONAL -DHAVE_MRUBY_REGEXP_GEM -DMRB_USE_SET -DMRB_USE_TASK_SCHEDULER src/string.c

# build_config/asan.rb
gcc -std=gnu99 -g -O3 -Wall -Wundef -Werror-implicit-function-declaration -Wwrite-strings -fsanitize=address,undefined -g3 -O0 -DMRB_DEBUG -DMRB_USE_BIGINT -DMRB_USE_COMPLEX -DHAVE_MRUBY_ENCODING_GEM -DMRB_UTF8_STRING -DHAVE_MRUBY_IO_GEM -DMRB_USE_RATIONAL -DHAVE_MRUBY_REGEXP_GEM -DMRB_USE_SET -DMRB_USE_TASK_SCHEDULER src/string.c

Summary by CodeRabbit

• Bug Fixes

  • Improved String#reverse! handling for UTF-8 strings.
  • Valid UTF-8 strings now reverse without breaking character boundaries or changing their character length.
  • Incomplete UTF-8 byte sequences are reversed correctly while retaining their invalid status.

• Tests

  • Added coverage for reversing both valid and incomplete UTF-8 strings.