UTF-8 validate strings before interning

[iluuu1994]

Fixes GH-10853

@alexdowad Sorry, I've only realized that you mentioned in #10409 that you'd like to work on this once I was already done.

I see it was also suggested that atomics could be used. I don't know C11 atomics at all but from what I've read it seems the type_info would have to be made atomic which is probably not a good idea as it's not usually necessary. But please correct me if I'm wrong. I'd also prefer if we didn't need to check all strings when interning, as it is only used in few cases atm and mbstrings implementation of UTF-8 validate is much more optimized.

[mvorisek]

https://github.com/alexdowad/php-src/blob/d882511808d973a354f63bb5821f552d46c09d8e/ext/mbstring/mbstring.c#L4621 should be used (and moved to non-mbstring/core if needed)

[mvorisek]

To fix #10853, a new flag should be added if the UTF-8 validity was checked or not, otherwise invalid UTF-8 string will need to be check on each call.

[alexdowad]

This looks great to me. Thanks.

The table-driven state machine for UTF-8 validity checking is very interesting. I'm interested to know how it compares when benchmarked against the 'fallback' UTF-8 validity checking implementation in mbstring (which was borrowed from PCRE).

[alexdowad]

To fix #10853, a new flag should be added if the UTF-8 validity was checked or not, otherwise invalid UTF-8 string will need to be check on each call.

👍

It looks like availability of bits in the object header should not be a problem... type_info, where the GC flags are kept, is 32 bits wide, and it looks like there are currently only 9 flags for zend_string objects.

[Girgias]

Minor nits, but LGTM.

Might make sense to pull the MBString UTF-8 checking code as it may be faster, but I'll let @alexdowad benchmark the different implementation :-)

[Girgias]

CS Nit:

Suggested change

	if (state == UTF8_REJECT)
	break;
	if (state == UTF8_REJECT) {
	break;
	}

[Girgias]

Doesn't seem to modify the pointer

Suggested change

	ZEND_API bool zend_string_validate_utf8(zend_string *string) {
	ZEND_API bool zend_string_validate_utf8(const zend_string *string) {

[youkidearitai]

I wrote a test script https://gist.github.com/youkidearitai/566e348e2e23301063ef5a95579d4efd(I put to ext/mbstring/tests) that this PR works fine. Would you like add this .phpt file?

[iluuu1994]

@alexdowad Did you have time benchmarking the two implementations? If either implementation is faster it might make sense to move that one to core and use it from mbstring. I'll also check whether we can see a performance hit during opcache persisting.

[bukka]

This is under BSD licence so it should be pointed here as well or ideally new header created so it doesn't mix licenses in the single file. I have been actually using this in jsond for some time and I separated the headers for that.

[alexdowad]

@alexdowad Did you have time benchmarking the two implementations?

Hi @iluuu1994. Sorry for the delay. I just put together a lil' benchmark program which runs both functions on a bunch of random strings and compares how long they take.

Using gcc with no optimization, the function from PCRE is somewhat faster. With gcc -O3... Bjoern's function is so ridiculously fast that it's hard to believe. I am just investigating.

[alexdowad]

Looks like compiler must have been optimizing calls to Bjoern's function out because the result was not used.

After making sure the result is used, the PCRE function is consistently faster.

[alexdowad]

Looks like @iluuu1994 would do better to use mb_fast_check_utf8_default from mbstring, or mb_fast_check_utf8_avx2 in cases where it can be used.

We already have a mechanism whereby Zend core calls into some mbstring function through function pointers which are set via zend_multibyte_set_functions... but I guess you won't want to use that for this purpose, because then you could only use the UTF-8 checking functions if mbstring extension is present.

I think you should go ahead and move both mb_fast_check_utf8_{default,avx2} into core, rename them (perhaps change mb_ to zend_), and expose them to callers in extension modules.

[alexdowad]

Maybe also rename _fast_check_ to just _check_.

[iluuu1994]

Thank you @alexdowad for your investigation! Moving the functions into core sounds good to me. I'll do that and check if there are any performance regressions for startup/compilation.