Major overhaul of mbstring (part 26) #9592

alexdowad · 2022-09-22T03:57:16Z

Use the new (faster) encoding conversion code for case conversion functions like mb_convert_case, mb_strtoupper, and mb_strtolower. Speed increase is only about 50% for title casing, but 2-3x for other types of case conversion.

Fuzzed with libfuzzer. One bug in my first draft of the implementation was found, and a regression test added.

Note: the signature of one function with public symbol (php_unicode_convert_case) is changed. This could break C extensions which link directly to mbstring and call this function. However, none of the PECL extensions do so.

FYA @cmb69 @nikic @kamil-tekiela

Perhaps @mvorisek might be interested. Recently he raised some suggestions about how to make mb_strtoupper and mb_strtolower faster. This PR does not close the performance gap with strtoupper and strtolower, but at least makes it much smaller than it was.

ext/mbstring/php_unicode.c

alexdowad · 2022-09-22T04:02:41Z

In case they are interested... @Girgias @kocsismate

Girgias · 2022-09-23T09:58:58Z

ext/mbstring/php_unicode.c

-};
-
-static int convert_case_filter(int c, void *void_data)
+MBSTRING_API zend_string *php_unicode_convert_case(int case_mode, const char *srcstr, size_t in_len, const mbfl_encoding *src_encoding, int illegal_mode, int illegal_substchar)


For the case mode, would it make sense to add an enum which contains all of the different modes?

And are the illegal_mode and illegal_substchar really ints? And maybe not bool, and a char? (Again I know very little about character encodings so the current API might be sensible)

Using an enum for case mode is a good idea.

illegal_mode is essentially an enum; there are 3-4 valid values. illegal_substchar is a Unicode codepoint, and therefore could be uint32_t.

Hmm. It seems that introducing an enum for illegal_mode may require reorganizing the header files for mbstring. Depending on which header file I define it in, there are a lot of issues with the order of header file inclusion.

I am just converting illegal_substchar to uint32_t.

For illegal_mode, I would like to handle this in the future. It seems to open a "can of worms" which is not directly related to this PR.

kamil-tekiela · 2022-09-23T21:11:25Z

Hi Alex,

I am doing some light testing and I cannot figure out why I get different outputs for mb_strtoupper() with your PR. I computed a diff of the codepoints, but I don't think these codepoints are valid Unicode characters, so I am a little confused why I got a mismatch. Is this something that your implementation changed?

Element 0 is input codepoint, element 2 is output codepoint.

array(40) {
  [0] =>
  array(2) {
    [0] =>
    int(11359)
    [2] =>
    int(11311)
  }
  [1] =>
  array(2) {
    [0] =>
    int(42945)
    [2] =>
    int(42944)
  }
  [2] =>
  array(2) {
    [0] =>
    int(42961)
    [2] =>
    int(42960)
  }
  [3] =>
  array(2) {
    [0] =>
    int(42967)
    [2] =>
    int(42966)
  }
  [4] =>
  array(2) {
    [0] =>
    int(42969)
    [2] =>
    int(42968)
  }
  [5] =>
  array(2) {
    [0] =>
    int(66967)
    [2] =>
    int(66928)
  }
  [6] =>
  array(2) {
    [0] =>
    int(66968)
    [2] =>
    int(66929)
  }
  [7] =>
  array(2) {
    [0] =>
    int(66969)
    [2] =>
    int(66930)
  }
  [8] =>
  array(2) {
    [0] =>
    int(66970)
    [2] =>
    int(66931)
  }
  [9] =>
  array(2) {
    [0] =>
    int(66971)
    [2] =>
    int(66932)
  }
  [10] =>
  array(2) {
    [0] =>
    int(66972)
    [2] =>
    int(66933)
  }
  [11] =>
  array(2) {
    [0] =>
    int(66973)
    [2] =>
    int(66934)
  }
  [12] =>
  array(2) {
    [0] =>
    int(66974)
    [2] =>
    int(66935)
  }
  [13] =>
  array(2) {
    [0] =>
    int(66975)
    [2] =>
    int(66936)
  }
  [14] =>
  array(2) {
    [0] =>
    int(66976)
    [2] =>
    int(66937)
  }
  [15] =>
  array(2) {
    [0] =>
    int(66977)
    [2] =>
    int(66938)
  }
  [16] =>
  array(2) {
    [0] =>
    int(66979)
    [2] =>
    int(66940)
  }
  [17] =>
  array(2) {
    [0] =>
    int(66980)
    [2] =>
    int(66941)
  }
  [18] =>
  array(2) {
    [0] =>
    int(66981)
    [2] =>
    int(66942)
  }
  [19] =>
  array(2) {
    [0] =>
    int(66982)
    [2] =>
    int(66943)
  }
  [20] =>
  array(2) {
    [0] =>
    int(66983)
    [2] =>
    int(66944)
  }
  [21] =>
  array(2) {
    [0] =>
    int(66984)
    [2] =>
    int(66945)
  }
  [22] =>
  array(2) {
    [0] =>
    int(66985)
    [2] =>
    int(66946)
  }
  [23] =>
  array(2) {
    [0] =>
    int(66986)
    [2] =>
    int(66947)
  }
  [24] =>
  array(2) {
    [0] =>
    int(66987)
    [2] =>
    int(66948)
  }
  [25] =>
  array(2) {
    [0] =>
    int(66988)
    [2] =>
    int(66949)
  }
  [26] =>
  array(2) {
    [0] =>
    int(66989)
    [2] =>
    int(66950)
  }
  [27] =>
  array(2) {
    [0] =>
    int(66990)
    [2] =>
    int(66951)
  }
  [28] =>
  array(2) {
    [0] =>
    int(66991)
    [2] =>
    int(66952)
  }
  [29] =>
  array(2) {
    [0] =>
    int(66992)
    [2] =>
    int(66953)
  }
  [30] =>
  array(2) {
    [0] =>
    int(66993)
    [2] =>
    int(66954)
  }
  [31] =>
  array(2) {
    [0] =>
    int(66995)
    [2] =>
    int(66956)
  }
  [32] =>
  array(2) {
    [0] =>
    int(66996)
    [2] =>
    int(66957)
  }
  [33] =>
  array(2) {
    [0] =>
    int(66997)
    [2] =>
    int(66958)
  }
  [34] =>
  array(2) {
    [0] =>
    int(66998)
    [2] =>
    int(66959)
  }
  [35] =>
  array(2) {
    [0] =>
    int(66999)
    [2] =>
    int(66960)
  }
  [36] =>
  array(2) {
    [0] =>
    int(67000)
    [2] =>
    int(66961)
  }
  [37] =>
  array(2) {
    [0] =>
    int(67001)
    [2] =>
    int(66962)
  }
  [38] =>
  array(2) {
    [0] =>
    int(67003)
    [2] =>
    int(66964)
  }
  [39] =>
  array(2) {
    [0] =>
    int(67004)
    [2] =>
    int(66965)
  }
}

Edit: I am running PHP 8.0 and while writing this, I realized I haven't compared it against 8.1. https://3v4l.org/M9sqM So it's not this PR.

alexdowad · 2022-09-23T22:05:26Z

@kamil-tekiela Thank you very much for testing! Could you share your test code?

kamil-tekiela · 2022-09-23T22:16:04Z

I only did a simple loop for ($i = 42; $i < 0x10FFFF; $i++) { and executed mb_strtoupper(mb_chr($i)); inside it. I then compared output on PHP 8.0 and with your patch. That's it. :)

alexdowad · 2022-09-23T22:18:30Z

@kamil-tekiela Then mb_strtoupper will use your default internal character encoding... what does mb_internal_encoding return on your system?

kamil-tekiela · 2022-09-23T23:00:02Z

It says UTF-8

alexdowad · 2022-10-04T06:26:42Z

@kamil-tekiela I am trying to investigate your findings, but am not getting the same results. Could you kindly share more of your testing code?

alexdowad · 2022-10-04T06:27:25Z

As an example:

<?php
var_dump(bin2hex(mb_strtoupper(mb_chr(11359))));

gives me:

string(6) "e2b0af"

alexdowad · 2022-10-04T06:29:04Z

@kamil-tekiela Sorry, I just saw the link you kindly provided to 3v4l. This is enough to work with.

alexdowad · 2022-10-04T06:33:15Z

Hmm. U+2C5F is Glagolitic Small Letter Caudate Chrivi, U+2C2F is Glagolitic Capital Letter Caudate Chivi. So it looks like PHP 8.1 is doing the right thing there.

Looks like this difference must be caused by fe36b81.

alexdowad · 2022-10-04T06:35:47Z

Just pushed 2 commits based on the helpful comments from @Girgias. Any further comments?

Thank you, everyone!

alexdowad · 2022-10-04T07:11:29Z

Failure on Travis is spurious (it's on a test for stat and lstat functions).

ext/mbstring/php_unicode.c

Girgias · 2022-10-04T12:34:26Z

From my PoV, this looks OK, but as I said far from an expert :)

…mb_strtolower Speed increase is only about 50% for title casing, but 2-3x for other forms of case conversion.

This value is a wchar, so the best type for it is uint32_t.

alexdowad · 2022-10-05T01:49:23Z

Merged. If anyone notices anything that can be improved later on, I can still revise these changes.

github-actions bot added the Extension: mbstring label Sep 22, 2022

alexdowad commented Sep 22, 2022

View reviewed changes

ext/mbstring/php_unicode.c Show resolved Hide resolved

Girgias reviewed Sep 23, 2022

View reviewed changes

Girgias reviewed Oct 4, 2022

View reviewed changes

ext/mbstring/php_unicode.c Outdated Show resolved Hide resolved

alexdowad added 3 commits October 5, 2022 05:05

Use fast text conversion filters for mb_convert_case, mb_strtoupper, …

e8e746c

…mb_strtolower Speed increase is only about 50% for title casing, but 2-3x for other forms of case conversion.

Make enum for valid case_mode values (for php_unicode_convert_case)

b4207d5

Use uint32_t for 'illegal_substchar' codepoint in mbstring

beae80b

This value is a wchar, so the best type for it is uint32_t.

alexdowad force-pushed the cleanup-mbstring-26 branch from c7b8cf7 to beae80b Compare October 4, 2022 20:06

alexdowad closed this Oct 5, 2022

Major overhaul of mbstring (part 26) #9592

Major overhaul of mbstring (part 26) #9592

Uh oh!

Conversation

alexdowad commented Sep 22, 2022

Uh oh!

Uh oh!

alexdowad commented Sep 22, 2022

Uh oh!

Girgias Sep 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexdowad Sep 23, 2022

Choose a reason for hiding this comment

Uh oh!

alexdowad Sep 24, 2022

Choose a reason for hiding this comment

Uh oh!

alexdowad Oct 4, 2022

Choose a reason for hiding this comment

Uh oh!

kamil-tekiela commented Sep 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexdowad commented Sep 23, 2022

Uh oh!

kamil-tekiela commented Sep 23, 2022

Uh oh!

alexdowad commented Sep 23, 2022

Uh oh!

kamil-tekiela commented Sep 23, 2022

Uh oh!

alexdowad commented Oct 4, 2022

Uh oh!

alexdowad commented Oct 4, 2022

Uh oh!

alexdowad commented Oct 4, 2022

Uh oh!

alexdowad commented Oct 4, 2022

Uh oh!

alexdowad commented Oct 4, 2022

Uh oh!

alexdowad commented Oct 4, 2022

Uh oh!

Uh oh!

Girgias commented Oct 4, 2022

Uh oh!

alexdowad commented Oct 5, 2022

Uh oh!

Uh oh!

Girgias Sep 23, 2022 •

edited

Loading

kamil-tekiela commented Sep 23, 2022 •

edited

Loading