Skip to content

gh-55531: Implement normalize_encoding in C #136643

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

StanFromIreland
Copy link
Member

@StanFromIreland StanFromIreland commented Jul 14, 2025

Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

I know that it's a draft but here are already some comments that you can dismiss if you're working on them.

@StanFromIreland
Copy link
Member Author

StanFromIreland commented Jul 14, 2025

I have cleaned up the changes and ensure the behavior remains the same, however there are still a few points I need input from @malemburg
(And as Benedikt said, should be their own issue)

  • This function is documented as taking strings, but during the 2->3 conversion and undocumented, and untested change was made which allowed it to accept bytes. I have kept it this way (in Python, to make removal simpler), though I think this should either be documented and tested, or removed.
  • The function has been documented as ascii only, and for bytes, it is. However, for strings, it has not been enforced with an error. What should we do?

@StanFromIreland StanFromIreland marked this pull request as ready for review July 14, 2025 12:54
@StanFromIreland StanFromIreland requested a review from picnixz July 14, 2025 12:54
Copy link
Member

@ZeroIntensity ZeroIntensity left a comment

Choose a reason for hiding this comment

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

Would you mind running some microbenchmarks?

@StanFromIreland
Copy link
Member Author

StanFromIreland commented Jul 15, 2025

Benchmarks:

script
import time
from encodings import normalize_encoding
import pyperf


def bench(loops):
    range_it = range(loops)
    t0 = time.perf_counter()

    for _ in range_it:
        normalize_encoding('utf_8')
        normalize_encoding('utf\xE9\u20AC\U0010ffff-8')
        normalize_encoding('utf   8')
        normalize_encoding('%%%~')
        normalize_encoding('UTF...8')

    return time.perf_counter() - t0


runner = pyperf.Runner()
runner.bench_time_func('normalize_encoding', bench, inner_loops=10)

Main branch:

normalize_encoding: Mean +- std dev: 173 ns +- 7 ns

This PR:

normalize_encoding: Mean +- std dev: 42.9 ns +- 1.1 ns

@malemburg
Copy link
Member

Sorry for the lack of response. I'm currently at EuroPython and pretty busy with other things. I'll have a look on Saturday during the sprints.

@serhiy-storchaka
Copy link
Member

There are some subtle differences between Python and C code. We first need to decide what normalization is needed in Python and C code. It seems that excessive normalization caused problems (see #88886).

@StanFromIreland
Copy link
Member Author

StanFromIreland commented Jul 21, 2025

There are some subtle differences between Python and C code.

All of our tests pass, and from my further testing it also matches behaviour, can you please point out such cases?

These are long standing issues that have had no progress for quite a while, I propose, to keep this PR simple/organized and therefore focused on switching the implementation to the existing C code. This PR will become more complex and therefore harder to review if it has to rewrite the existing C code too. The existing issues, and yours from a few days ago, can be addressed in the C implementation, rather than both implementations.

@serhiy-storchaka
Copy link
Member

I suggest holding this PR until we solve other issues. Otherwise it will make backporting other changes more difficult.

I have some comments about this PR, but it's too early to address them because in the end, everything could change radically.

@bedevere-app
Copy link

bedevere-app bot commented Jul 22, 2025

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@StanFromIreland
Copy link
Member Author

I have made the requested changes; please review again

@bedevere-app
Copy link

bedevere-app bot commented Jul 27, 2025

Thanks for making the requested changes!

@malemburg: please review the changes made to this pull request.

@bedevere-app bedevere-app bot requested a review from malemburg July 27, 2025 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants