ext/mbstring: Optimize valid UTF-8 in mb_substr()

[kamil-tekiela]

This PR adds a fast path for valid UTF-8 strings in mb_substr. It skips bytes in 256 chunks. The performance improvement is noticeable with longer strings and only ones that are flagged as valid UTF-8.

@alexdowad What do you think? Is this worth doing? I haven't got a fuzzer, but I don't anticipate any behavioural changes.

[alexdowad]

@kamil-tekiela Fuzzing is 100% necessary. Otherwise, it is very, very easy for subtle bugs to slip through.

It's good that you optimized c < 0x80 || (c & 0xC0) != 0x80 to just (c & 0xC0) != 0x80; I think that change alone can go direct to master.

[kamil-tekiela]

Do you have a fuzzer available? If not can you give me hints on how to build one?

[alexdowad]

@kamil-tekiela When I have some time, I could fuzz this for you. Or, if you want to do it yourself, that might be a very good idea... fuzzing is an extremely powerful technique for finding bugs, and is an awesome tool for any programmer to know how to use.

I once wrote up my workflow for fuzzing new code in mbstring here: #10828 (comment)

[kamil-tekiela]

I tried fuzzing as you said and it didn't find anything. Not sure I've done it properly, but when I intentionally introduced a bug it found it immediately.

[alexdowad]

@kamil-tekiela OK, that is a good sign! However, how meaningful it is depends on what you asserted in the fuzzer. At the least, this shows that you probably don't have memory corruption bugs, which is good. But we can check more.

A powerful way to test refactoring/optimization changes like this is: cut-and-paste the original code for the function into your mbstring.c file, and give the old function a different name, perhaps like mb_get_strpos_OLD and mb_find_substr_OLD. Make the fuzzer:

1. Generate random inputs
2. Call both the old and new versions of the same function
3. Assert that the outputs are exactly the same

Then when you run the fuzzer, it will search for inputs which produce different results from the old and new implementations of the same function.

[kamil-tekiela]

@alexdowad Thanks. I run the fuzzer as you suggested now and it found one regression, but I cannot understand why it works that way. https://3v4l.org/Q8OB3#

Until PHP 8.3.1, it worked the way that my optimisation works but since then it returns a different output. I think I can restrict my optimization to skip DOCOMO encoding, but I don't understand why there's a difference, so now I doubt whether my optimization is correct for other UTF-8 encodings too.

The https://www.php.net/ChangeLog-8.php#8.3.2 doesn't mention any bug fix in mbstring either.

[youkidearitai]

I don't know that details, PHP 8.3 was overhaul mbstring(https://github.com/php/php-src/pulls?q=is%3Apr+major+overhaul+of). Maybe there is no changelogs.

[youkidearitai]

Ah, I got it.

U+00AE (®、Registerd trade mark) is C2AE in UTF-8, But UTF-8 DoCoMo is convert to EE9CB6.
Please see(I suggest use CTRL+F command): https://punchdrunker.github.io/iOSEmoji/table_html/ios6/number.html

Therefore, PHP 8.3.2 and younger(8.3.2-master) is correct.

[youkidearitai]

git bisect is end. Commit hash is ec348a1.

[kamil-tekiela]

Ah, I got it.

U+00AE (®、Registerd trade mark) is C2AE in UTF-8, But UTF-8 DoCoMo is convert to EE9CB6. Please see(I suggest use CTRL+F command): https://punchdrunker.github.io/iOSEmoji/table_html/ios6/number.html

Therefore, PHP 8.3.2 and younger(8.3.2-master) is correct.

Maybe, it's correct, but it's not very consistent https://3v4l.org/0D7hG#v

[ArtUkrainskiy]

This needs a fix before merge: the fast path is also taken for the UTF-8-Mobile encodings, and for them the result now depends on whether the string was checked before.

This is the difference your fuzzer found, and it applies to all four variants that php_mb_is_no_encoding_utf8() accepts: UTF-8-Mobile#DOCOMO, #KDDI-A, #KDDI-B and #SOFTBANK. Since ec348a1 (PHP 8.3.2, the commit @youkidearitai bisected to), mb_substr() decodes and encodes again for them, which turns U+00AE into the carrier's code point; before that commit it copied bytes using the mblen_table, which is why PHP 8.2 and 8.3.0–8.3.1 give c2ae. With the PR, a string with the valid-UTF-8 flag skips the conversion, and a negative offset is enough to set the flag:

$s = str_repeat("a\u{AE}b", 2);
var_dump(bin2hex(mb_substr($s, 1, 1, 'UTF-8-Mobile#DOCOMO'))); // "ee9cb6"
mb_substr($s, -1, null, 'UTF-8-Mobile#DOCOMO'); // counts the length, which marks $s as valid UTF-8
var_dump(bin2hex(mb_substr($s, 1, 1, 'UTF-8-Mobile#DOCOMO'))); // master: "ee9cb6", PR: "c2ae"

On master, mb_str_split() and mb_strcut() already return the input bytes for all four variants, while mb_substr(), mb_strstr() and mb_convert_encoding() encode again, so the inconsistency you noticed is older than this PR. Which behaviour is right for the mobile variants is a separate question. Keeping them on the decoding path (suggestion below) makes the output the same as master in all my checks.

[ArtUkrainskiy]

I measured the current head 6d9250e on master 1c9242e (it merges cleanly) with GCC 13.3, GCC 14.2, clang 18.1 and clang 20.1 on an i7-13700H, built with the flags of the official Docker image. With the UTF-8-only condition the UTF-8-Mobile difference from my review is gone.

Change in CPU cycles per call with the PR, relative to master (negative = fewer cycles, faster); median of five alternating runs. The PR only changes strings with the internal valid-UTF-8 flag, so the rows run on a string that went through mb_check_encoding($s, 'UTF-8') once before the loop, except the three marked "without the flag". Each language gets the same calls on generated text; B/char is the bytes per character of that input:

call	master, CPU cycles per call (GCC 13.3)	GCC 13.3	GCC 14.2	clang 18.1	clang 20.1
mb_substr($s, 0, 10), English (1.00 B/char), 31-byte title	200	−33%	−21%	−46%	−47%
mb_substr($s, $half, 20), English (1.00 B/char), 1 KB	1,366	−82%	−81%	−87%	−87%
mb_substr($s, $half, 20), English (1.00 B/char), 64 KB	63,324	−92%	−90%	−95%	−95%
mb_substr($s, 0, 10), Spanish (1.00 B/char), 37-byte title	210	−37%	−25%	−47%	−50%
mb_substr($s, $half, 20), Spanish (1.04 B/char), 1 KB	1,620	−63%	−57%	−76%	−71%
mb_substr($s, 0, 10), Russian (1.81 B/char), 49-byte title	275	−37%	−40%	−58%	−56%
mb_substr($s, $half, 20), Russian (1.78 B/char), 1 KB	1,720	−44%	−44%	−70%	−72%
mb_substr($s, 0, 10), Arabic (1.75 B/char), 56-byte title	302	−45%	−45%	−60%	−60%
mb_substr($s, $half, 20), Arabic (1.74 B/char), 1 KB	1,868	−43%	−42%	−70%	−68%
mb_substr($s, 0, 10), Hindi (2.61 B/char), 94-byte title	475	−60%	−60%	−67%	−68%
mb_substr($s, $half, 20), Hindi (2.54 B/char), 1 KB	2,191	−18%	−10%	−43%	−43%
mb_substr($s, 0, 10), Chinese (3.00 B/char), 54-byte title	389	−46%	−46%	−58%	−58%
mb_substr($s, $half, 20), Chinese (2.89 B/char), 1 KB	2,830	−47%	−34%	−51%	−56%
mb_substr($s, 0, 10), Japanese (3.00 B/char), 63-byte title	406	−49%	−48%	−61%	−61%
mb_substr($s, $half, 20), Japanese (2.91 B/char), 1 KB	2,749	−48%	−28%	−50%	−56%
mb_substr($s, 0, 10), Korean (2.33 B/char), 49-byte title	355	−47%	−49%	−58%	−57%
mb_substr($s, $half, 20), Korean (2.24 B/char), 1 KB	3,023	−41%	−32%	−62%	−57%
mb_substr($s, 0, 10), English with emoji (1.30 B/char), 26-byte title	176	−24%	−26%	−42%	−40%
mb_substr($s, $half, 20), English with emoji (1.39 B/char), 1 KB	1,982	−50%	−46%	−68%	−68%
mb_substr($s, -20), English (1.00 B/char), 1 KB	2,310	−66%	−62%	−70%	−67%
mb_substr($s, 0, $half), English (1.00 B/char), 1 KB	2,079	−89%	−88%	−92%	−92%
mb_strpos($s, $needle, $half), English (1.00 B/char), 1 KB	6,290	−91%	−90%	−92%	−92%
mb_substr($s, -20), English (1.00 B/char), 64 KB	134,526	−87%	−84%	−91%	−91%
mb_strpos($s, $needle, $half), English (1.00 B/char), 64 KB	385,906	−93%	−93%	−94%	−94%
mb_substr($s, 0, 10), English (1.00 B/char), 31-byte title, without the flag	199	+4%	+3%	0%	+1%
mb_substr($s, $half, 20), English (1.00 B/char), 1 KB, without the flag	1,363	−1%	+1%	0%	−1%
mb_substr($s, $half, 20), Chinese (2.89 B/char), 1 KB, without the flag	2,841	0%	+1%	0%	0%

• With the flag: 21–68% fewer cycles for the first 10 characters of a title in all nine languages, 90–98% for 20 characters from the middle of 64 KB (the report has every language at 64 KB). From the middle of 1 KB it depends on the bytes per character: 81–87% in English, 32–76% in Spanish, Russian, Arabic, Korean and with emoji, 28–56% in Chinese and Japanese, 10–43% in Hindi.
• Without the flag: −1 … +4% on all four compilers, from the added flag check (6–18 more instructions per call); master against master in the same series −2 … +3%.
• A string gets the flag from mb_check_encoding(), mb_strlen(), preg_match()/preg_match_all() with /u from offset 0, and from mb_substr() itself with a negative offset or length (it counts the length first), so mb_substr($s, -20) takes the new path on any valid UTF-8 string. Slices from the new path keep the flag.

Correctness: I compared the output with master for the functions that go through mb_get_substr() or mb_find_strpos() with the string itself (mb_substr(), the mb_str*pos() and mb_str*chr()/mb_str*str() families, mb_strimwidth(), mb_ucfirst()/mb_lcfirst(), mb_*trim()), on random strings with 1- to 4-byte characters (carrier emoji ranges, flags, keycaps, 10% invalid), checked and unchecked, many of them around multiples of 256 characters long, in UTF-8 and the four UTF-8-Mobile encodings, plus mb_strpos()/mb_stripos()/mb_strrpos()/mb_substr() on the same strings in SJIS, UTF-16LE, ISO-8859-1 and EUC-JP. With the current head (I tested the same code as my suggestion on top of the first version) the output is the same as master in all of them: 40,000 strings with GCC 13.3 and 3,000 under ASan/UBSan. The first version, b218ba6, gave the same UTF-8 output as master on all four compilers as well (40,000 with GCC 13.3, 10,000 with each of the others). The check catches seven of eight deliberate mutations of the new code.

Code: I found nothing to change. The chunk loop runs only while offset > 256, so it cannot take the offset below zero; a chunk that ends inside a character is fine because the byte loop after it skips continuation bytes; and flagging the result keeps chained calls on the fast path. The offset > 256 limit is also why Hindi, Chinese, Japanese and Korean gain least at 1 KB: their middle is 181–241 characters, so it is counted byte by byte. A 256-byte chunk of 2- to 4-byte text holds 64–128 characters, so I tried two ways to use the chunks below 256, as a possible follow-up. Counting each chunk and skipping it while it holds no more characters than the offset makes those four rows 66–77% faster than the PR, but English 1 KB slower: 25–52% from the middle, 22–43% for the first half, 7–10% for mb_strpos(). Not counting below 64 characters keeps English 1 KB from the middle within 2% of the PR with GCC 14.2 only, and makes the mb_substr() rows on 64 KB 12–64% slower with clang. (Both measured against the first version, whose UTF-8 path is the same as the current head's.) So I would leave the PR as it is; the numbers for both are in the report.

Tests: the PR adds none, and ext/mbstring/tests passed with the first version, so they did not see the UTF-8-Mobile issue; they also miss two mutations of the chunk loop (offset > 0 instead of offset > 256, a 255-byte step). This test fails with the first version, passes on master and with the current head, and catches those three:

--TEST--
mb_substr() gives the same result for strings already checked as valid UTF-8
--EXTENSIONS--
mbstring
--FILE--
<?php
// 1-, 2-, 3- and 4-byte characters, 400 characters in 1000 bytes
function make() { return str_repeat("a\u{E9}\u{20AC}\u{1F600}", 100); }

foreach ([0, 1, 255, 256, 257, 399, 400, 401, -1, -257] as $from) {
    foreach ([null, 0, 1, 300, -1] as $len) {
        $checked = make();
        mb_check_encoding($checked, 'UTF-8');
        if (mb_substr(make(), $from, $len, 'UTF-8') !== mb_substr($checked, $from, $len, 'UTF-8')) {
            echo "differs: from $from, length ", var_export($len, true), "\n";
        }
    }
}

// The UTF-8-Mobile encodings convert U+00AE to a carrier code point, checked or not
foreach (['UTF-8-Mobile#DOCOMO', 'UTF-8-Mobile#KDDI-A', 'UTF-8-Mobile#SOFTBANK'] as $enc) {
    $s = str_repeat("a\u{AE}b", 2);
    $before = bin2hex(mb_substr($s, 1, 1, $enc));
    mb_check_encoding($s, 'UTF-8');
    var_dump($before, bin2hex(mb_substr($s, 1, 1, $enc)));
}
?>
--EXPECT--
string(6) "ee9cb6"
string(6) "ee9cb6"
string(6) "ee9599"
string(6) "ee9599"
string(6) "ee898f"
string(6) "ee898f"

No correctness issues found. LGTM.

Full tables, scripts and data: https://github.com/ArtUkrainskiy/php-src-bench/tree/main/reports/pr-24080-mb-substr