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-89653: PEP 670: Convert PyUnicode_KIND() macro to function by vstinner · Pull Request #92705 · python/cpython

Conversation

@vstinner

@vstinner vstinner commented May 12, 2022

Copy link
Copy Markdown
Member

In the limited C API version 3.12, PyUnicode_KIND() is now
implemented as a static inline function. Keep the macro for the
regular C API and for the limited C API version 3.11 and older to
prevent introducing new compiler warnings.

Update _decimal.c and stringlib/eq.h for PyUnicode_KIND().

Sorry, something went wrong.

Uh oh!

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

All reactions
@vstinner
vstinner requested a review from rhettinger as a code owner May 12, 2022 00:33
@bedevere-bot bedevere-bot added the awaiting core review label May 12, 2022
@vstinner vstinner added the skip news label May 12, 2022
@vstinner vstinner mentioned this pull request May 12, 2022
gh-89653: Use int type for Unicode kind #92704
Merged
@vstinner

vstinner commented May 12, 2022

Copy link
Copy Markdown
Member Author

@serhiy-storchaka @methane @erlend-aasland: I propose to define the PyUnicode_KIND() result type as int. Are you ok with that?

My first motivation is that the only existing function using a kind is PyUnicode_FromKindAndData() and the kind parameter type is int.

The int type is very common in C and easy to use. IMO using unsigned int is less convenient. Enums must be avoided in the C API, they are not portable and cause issues in programming languages other than C: so enum PyUnicode_Kind cannot be used.

Previously, the exact C type of the PyUnicode_KIND() result was never well defined. It was documented as unsigned int. But int (signed) and enum PyUnicode_Kind were also accepted and didn't emit any compiler warning (even with -Wconversion).

I also prepared the PR #92704 which defines all "kind" variables as int.

I already merged the commit d0c9353 which prepared the code and the API.

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner

vstinner commented May 12, 2022

Copy link
Copy Markdown
Member Author

This PR only impacts C extensions which opt-in for the limited C API version 3.12 and newer. Other C extensions are not affected (no change).

All reactions

Sorry, something went wrong.

Uh oh!

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

erlend-aasland
erlend-aasland reviewed May 12, 2022
View reviewed changes
Comment thread Objects/stringlib/eq.h Resolved

Uh oh!

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

erlend-aasland
erlend-aasland approved these changes May 12, 2022
View reviewed changes

@erlend-aasland erlend-aasland left a comment

Copy link
Copy Markdown
Contributor

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.

Looks good to me

Sorry, something went wrong.

Uh oh!

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

All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels May 12, 2022
@vstinner
gh-89653: PEP 670: Convert PyUnicode_KIND() macro to function
3bce0ec
In the limited C API version 3.12, PyUnicode_KIND() is now
implemented as a static inline function. Keep the macro for the
regular C API and for the limited C API version 3.11 and older to
prevent introducing new compiler warnings.

Update _decimal.c and stringlib/eq.h for PyUnicode_KIND().
@vstinner

vstinner commented May 12, 2022

Copy link
Copy Markdown
Member Author

PR rebased to fix merge conflicts.

All reactions

Sorry, something went wrong.

Uh oh!

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

erlend-aasland
erlend-aasland approved these changes May 12, 2022
View reviewed changes
methane
methane approved these changes May 12, 2022
View reviewed changes
@vstinner
vstinner merged commit db388df into python:main May 13, 2022
@bedevere-bot bedevere-bot removed the awaiting merge label May 13, 2022
@vstinner
vstinner deleted the kind_func branch May 13, 2022 09:50
@vstinner

vstinner commented May 13, 2022

Copy link
Copy Markdown
Member Author

Thanks for your reviews @methane and @erlend-aasland, I really wasn't sure about the PyUnicode_KIND() return type.

Next: deprecate the whole API which expose kind+data, it's too low-level, too specific to PEP 393. I seriously consider suggesting higher level APIs. It would help to experiment using UTF-8 internally in CPython, as PyPy does (with an index to keep O(1) complexity for str[index] character index).

All reactions

Sorry, something went wrong.

Uh oh!

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

@serhiy-storchaka

serhiy-storchaka commented May 13, 2022

Copy link
Copy Markdown
Member

I predict a serious performance hit in modules which parse strings (like re or json, or third-party alternatives) if we get rid of this API.

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner

vstinner commented May 13, 2022

Copy link
Copy Markdown
Member Author

I predict a serious performance hit in modules which parse strings (like re or json, or third-party alternatives) if we get rid of this API.

The API would be part of the internal C API and so can be used by stdlib extensions, even if they are built as shared libraries.

Yeah, the performance overhead of 3rd party C extensions should be measured to consider the idea.

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner

vstinner commented May 13, 2022

Copy link
Copy Markdown
Member Author

I have 2 motivations to hide the (kind, data) low-level API:

  • It prevents CPython to use something else, like UTF-8
  • Other Python implementations must emulate it in their implementation of the Python C API: that's annoying since the API has no function to say "i'm done with modifying the mutable strings: convert it to a final immutable object". The whole API is based on the assumption that we can freely modify mutable strings with the assumption that it's not possible to access this strings that we are building.

The _PyUnicodeWriter private API has a _PyUnicodeWriter_Finish() function which builds the final immutable string. It function calls realloc() if the buffer was overallocated.

I would prefer to expose such "writer" API for 3rd party C extensions.

All reactions

Sorry, something went wrong.

Uh oh!

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

@methane

methane commented May 13, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

Perf hit would be measurable, but not end of the world.

In 2018, I port MarkupSafe to PEP 393. I wrote logic in macro and 3 functions for each kind.
It is very difficult to maintain.
https://github.com/pallets/markupsafe/pull/64/files

In 2020, I port Genshi to Python 3.3+, but used UTF-8 instead of PEP 393 API.
It looks like regular string processing C code.
https://github.com/edgewall/genshi/pull/26/files

I think optimizing for PEP 393 won't pay for most cases.

All reactions

Sorry, something went wrong.

Uh oh!

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

@serhiy-storchaka

serhiy-storchaka commented May 13, 2022

Copy link
Copy Markdown
Member

For re, the difference was enormous, up to 4 times for non-ASCII data in microbenchmarks and up to 25% in standard pure ASCII benchmarks. See #62885. This may be a reason of regex been ~25% slower than re in many benchmarks.

I would not want to lose such advantage.

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner

vstinner commented May 13, 2022

Copy link
Copy Markdown
Member Author

The _PyUnicodeWriter API should be as efficient as using the (kind, data) API. Overallocation can be enabled or disabled, and it's possible to request max character when the writer is initialized to avoid expensive conversions from UCS1 to UCS2 and then to UCS4. But it's usually hard to know in advance to know what will be the final maximum character. _PyUnicodeWriter also allows to change the buffer format at each write from UCS1, to UCS2 and then UCS4.

In the early days of Python 3.3, most code only used UCS4 buffers (simple C array of Py_UCS4 characters allocated on the heap) and then downcasted to UCS1 or UCS2 when the final string was created.

I'm not proposing to expose directly the _PyUnicodeWriter API. I'm thinking about proposing a public C API with the same idea: "append only" API and a "finish" function which returns a Python str object.

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner

vstinner commented May 13, 2022

Copy link
Copy Markdown
Member Author

For re, the difference was enormous, up to 4 times for non-ASCII data in microbenchmarks and up to 25% in standard pure ASCII benchmarks. See #62885. This may be a reason of regex been ~25% slower than re in many benchmarks.

Would it be acceptable for the regex module to use the internal C API to access directly to the Python str internals: the (kind, data) API?

Anyway, this idea is in the PEP area: use cases should be written down, and the performance of each use case should be measured.

If possible, I would prefer to have no impact or minor overhead on CPython, but the main goal is to get "native speed" for other Python implementations, especially PyPy which uses UTF-8 internally.

All reactions

Sorry, something went wrong.

Uh oh!

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

@erlend-aasland

erlend-aasland commented May 13, 2022

Copy link
Copy Markdown
Contributor

Can we move this discussion to Discourse or a separate issue?

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner

vstinner commented May 16, 2022

Copy link
Copy Markdown
Member Author

The future of the (kind, data) API is discussed at: https://discuss.python.org/t/un-deprecate-pyunicode-ready-for-future-unicode-improvement/15717

erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@methane methane methane approved these changes
@erlend-aasland erlend-aasland erlend-aasland approved these changes
@rhettinger rhettinger Awaiting requested review from rhettinger

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.

5 participants

@vstinner @serhiy-storchaka @methane @erlend-aasland @bedevere-bot
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 01402503 — 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.