GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-120196: Reuse find_max_char for bytes objects by rhpvorderman · Pull Request #120497 · python/cpython

Conversation

@rhpvorderman

@rhpvorderman rhpvorderman commented Jun 14, 2024 •
edited by bedevere-app Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Simple duplicate code removal and reuse of other function.

  • Issue: Faster decode and find_max_char implementations. #120196

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@rhpvorderman
Reuse find_max_char for bytes objects
59bbd60
@bedevere-app bedevere-app Bot added the awaiting review label Jun 14, 2024
@rhpvorderman rhpvorderman mentioned this pull request Jun 14, 2024
gh-120196: Faster ascii_decode and find_max_char implementations #120212
Closed
vstinner
vstinner reviewed Jun 14, 2024
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@rhpvorderman

rhpvorderman commented Jun 14, 2024

Copy link
Copy Markdown
Contributor Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Jun 14, 2024

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@rhpvorderman
Fix compiler warnings
5852ce3
@rhpvorderman

rhpvorderman commented Jun 14, 2024

Copy link
Copy Markdown
Contributor Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vstinner
vstinner approved these changes Jun 14, 2024
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels Jun 14, 2024
@serhiy-storchaka

serhiy-storchaka commented Jun 14, 2024

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@rhpvorderman

rhpvorderman commented Jun 14, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Jun 14, 2024

Copy link
Copy Markdown
Member

What with the new implementation?

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@serhiy-storchaka

serhiy-storchaka commented Jun 14, 2024

Copy link
Copy Markdown
Member

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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@rhpvorderman

rhpvorderman commented Jun 14, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

rhpvorderman
rhpvorderman commented Jun 14, 2024
View reviewed changes
Comment thread Objects/bytes_methods.c
Return True if B is empty or all characters in B are ASCII,\n\
False otherwise.");

// Optimization is copied from ascii_decode in unicodeobject.c

@rhpvorderman rhpvorderman Jun 14, 2024

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@rhpvorderman

rhpvorderman commented Jun 17, 2024

Copy link
Copy Markdown
Contributor Author

@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).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Jun 17, 2024

Copy link
Copy Markdown
Member

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

Thanks for actually checking.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner vstinner changed the title Reuse find_max_char for bytes objects gh-120196: Reuse find_max_char for bytes objects Jun 17, 2024
@bedevere-app bedevere-app Bot mentioned this pull request Jun 17, 2024
Faster decode and find_max_char implementations. #120196
Closed
@vstinner vstinner changed the title gh-120196: Reuse find_max_char for bytes objects gh-120196: Reuse find_max_char for bytes objects Jun 17, 2024
@vstinner vstinner added the skip news label Jun 17, 2024
serhiy-storchaka
serhiy-storchaka approved these changes Jun 17, 2024
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@vstinner
vstinner merged commit 945a89b into python:main Jun 17, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label Jun 17, 2024
@vstinner

vstinner commented Jun 17, 2024

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@rhpvorderman

rhpvorderman commented Jun 17, 2024

Copy link
Copy Markdown
Contributor Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@rhpvorderman
rhpvorderman deleted the bytesasciiredundant branch June 17, 2024 11:17
mrahtz pushed a commit to mrahtz/cpython that referenced this pull request Jun 30, 2024
@rhpvorderman @mrahtz
pythongh-120196: Reuse find_max_char() for bytes objects (python#120497)
b93d655
estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
@rhpvorderman @estyxx
pythongh-120196: Reuse find_max_char() for bytes objects (python#120497)
9d3b846
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner approved these changes
@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@rhpvorderman @vstinner @serhiy-storchaka
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.