gh-120196: Reuse find_max_char for bytes objects

[rhpvorderman]

Simple duplicate code removal and reuse of other function.

• Issue: Faster decode and find_max_char implementations. #120196

[vstinner]

There are many "comparison of distinct pointer types lacks a cast" compiler warnings. Can you have a look? (Look at the PR to see the warnings.)

[rhpvorderman]

Ah, shoot. This has to do with the current implementation, and I was working in a different implementation in #120196 . Is it OK to wait for that one to be merged/rejected? Otherwise there will be some annoying merge conflicts that will take extra time.

[vstinner]

It's up to you. This change looks simpler to be merged than the other one which looks controversial.

[rhpvorderman]

Done. It was an easy change and it makes that the compile guards above are indeed applicable because there is no mixing of signed and unsigned pointer types.

[vstinner]

LGTM.

@pitrou @serhiy-storchaka: Would you mind to review this change?

[serhiy-storchaka]

Currently bytes.isascii() returns as soon as it finds the first non-ASCII byte. What with the new implementation?

[rhpvorderman]

Same. Or technically 2 clock cycles later as it will evaluate whether the return value is larger than 127, but if it is properly inlined it should be 0 clock cycles.

EDIT: Sorry, I should have left out the mumbo jumbo. Find_max_char also exits early when it finds a non-ascii character.

[vstinner]

What with the new implementation?

It's about reusing existing code, IMO it's a good thing.

[serhiy-storchaka]

Could you please provide microbenchmark results for short and long, ASCII and non-ASCII (with non-ASCII bytes at the begin and at the end) byte strings?

[rhpvorderman]

There is no need for benchmarking. It is literally the same code. This is removing a copy paste with a function call. See below.

This is the old duplicated code

PyObject*
_Py_bytes_isascii(const char *cptr, Py_ssize_t len)
{
    const char *p = cptr;
    const char *end = p + len;

    while (p < end) {
        /* Fast path, see in STRINGLIB(utf8_decode) in stringlib/codecs.h
           for an explanation. */
        if (_Py_IS_ALIGNED(p, ALIGNOF_SIZE_T)) {
            /* Help allocation */
            const char *_p = p;
            while (_p + SIZEOF_SIZE_T <= end) {
                size_t value = *(const size_t *) _p;
                if (value & ASCII_CHAR_MASK) {
                    Py_RETURN_FALSE;
                }
                _p += SIZEOF_SIZE_T;
            }
            p = _p;
            if (_p == end)
                break;
        }
        if ((unsigned char)*p & 0x80) {
            Py_RETURN_FALSE;
        }
        p++;
    }
    Py_RETURN_TRUE;
}

This is the code that PR is now calling

Py_LOCAL_INLINE(Py_UCS4)
STRINGLIB(find_max_char)(const STRINGLIB_CHAR *begin, const STRINGLIB_CHAR *end)
{
    const unsigned char *p = (const unsigned char *) begin;
    const unsigned char *_end = (const unsigned char *)end;

    while (p < _end) {
        if (_Py_IS_ALIGNED(p, ALIGNOF_SIZE_T)) {
            /* Help register allocation */
            const unsigned char *_p = p;
            while (_p + SIZEOF_SIZE_T <= _end) {
                size_t value = *(const size_t *) _p;
                if (value & UCS1_ASCII_CHAR_MASK)
                    return 255;
                _p += SIZEOF_SIZE_T;
            }
            p = _p;
            if (p == _end)
                break;
        }
        if (*p++ & 0x80)
            return 255;
    }
    return 127;
}

EDIT: And because it is inlined, the compiler can switch the return 255 statements with Py_RETURN_TRUE statements. It should result in the same assembly.

[rhpvorderman]

Rather than being a copy, an inline function call to virtually the same code is now used.

[rhpvorderman]

@serhiy-storchaka Sorry, I didn't mean to be so abrasive. Here are the microbenchmarks:

Before:

$ python -m timeit -s 'data=b"longstringallascii" *2000' 'data.isascii()'
100000 loops, best of 5: 2.4 usec per loop
$ python -m timeit -s 'data=b"\xff" + b"longstringnotascii" *2000 ' 'data.isascii()'
20000000 loops, best of 5: 17.2 nsec per loop
$ python -m timeit -s 'data=b"longstringnotascii" *2000 + b"\xff"' 'data.isascii()'
100000 loops, best of 5: 2.41 usec per loop
$ python -m timeit -s 'data=b"shortstringallascii"' 'data.isascii()'
20000000 loops, best of 5: 20.5 nsec per loop
$ python -m timeit -s 'data=b"\xffhortstringnotascii"' 'data.isascii()'
20000000 loops, best of 5: 17.3 nsec per loop
$ python -m timeit -s 'data=b"shortstringnotascii/xff"' 'data.isascii()'
10000000 loops, best of 5: 22.4 nsec per loop

After

$ python -m timeit -s 'data=b"longstringallascii" *2000' 'data.isascii()'
100000 loops, best of 5: 2.4 usec per loop
$ python -m timeit -s 'data=b"\xff" + b"longstringnotascii" *2000 ' 'data.isascii()'
20000000 loops, best of 5: 17.1 nsec per loop
$ python -m timeit -s 'data=b"longstringnotascii" *2000 + b"\xff"' 'data.isascii()'
100000 loops, best of 5: 2.4 usec per loop
$ python -m timeit -s 'data=b"shortstringallascii"' 'data.isascii()'
20000000 loops, best of 5: 20 nsec per loop
$ python -m timeit -s 'data=b"\xffhortstringnotascii"' 'data.isascii()'
20000000 loops, best of 5: 17 nsec per loop
$ python -m timeit -s 'data=b"shortstringnotascii/xff"' 'data.isascii()'
10000000 loops, best of 5: 22.4 nsec per loop

As expected, the results are the same (within run-to-run expected error).

[vstinner]

As expected, the results are the same (within run-to-run expected error).

Thanks for actually checking.

[serhiy-storchaka]

Thank you. It is always better to check even if the answer looks obvious. We can miss some details, and the compiler can produce unexpected results.

LGTM.

[vstinner]

Merged, thank you. Oops, I merged the PR while @serhiy-storchaka was reviewing it :-)

[rhpvorderman]

Thank you. It is always better to check even if the answer looks obvious. We can miss some details, and the compiler can produce unexpected results.

I will keep that in mind for next time. Thanks for reviewing, both of you.