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-55531: Implement `normalize_encoding` in C by StanFromIreland · Pull Request #136643 · python/cpython

Conversation

@StanFromIreland

@StanFromIreland StanFromIreland commented Jul 14, 2025 •
edited by bedevere-app Bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member
  • Issue: encoding package's normalize_encoding() function is too slow #55531

Sorry, something went wrong.

Uh oh!

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

All reactions
@StanFromIreland
C
92873d6
@StanFromIreland
StanFromIreland requested review from malemburg and vstinner July 14, 2025 08:35
@bedevere-app bedevere-app Bot mentioned this pull request Jul 14, 2025
encoding package's normalize_encoding() function is too slow #55531
Closed
StanFromIreland
StanFromIreland commented Jul 14, 2025
View reviewed changes
Comment thread Lib/encodings/__init__.py Resolved

Uh oh!

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

Comment thread Lib/test/test_codecs.py Resolved

Uh oh!

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

Comment thread Objects/unicodeobject.c Resolved

Uh oh!

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

@StanFromIreland
Correct clinic note
4bae23a
picnixz
picnixz reviewed Jul 14, 2025
View reviewed changes

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

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

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Lib/encodings/__init__.py Resolved

Uh oh!

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

Comment thread Lib/test/test_codecs.py Resolved

Uh oh!

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

Comment thread Modules/_codecsmodule.c Resolved

Uh oh!

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

Comment thread Objects/unicodeobject.c Resolved

Uh oh!

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

Comment thread Modules/_codecsmodule.c Resolved

Uh oh!

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

Comment thread Modules/_codecsmodule.c Resolved

Uh oh!

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

Comment thread Lib/encodings/__init__.py Resolved

Uh oh!

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

StanFromIreland added 2 commits July 14, 2025 09:54
@StanFromIreland
Little fixes
b5f3df3
@StanFromIreland
Keep the messiness
2ad72b2
picnixz
picnixz reviewed Jul 14, 2025
View reviewed changes
Comment thread Modules/_codecsmodule.c Resolved

Uh oh!

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

@StanFromIreland

StanFromIreland commented Jul 14, 2025 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

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?
All reactions

Sorry, something went wrong.

Uh oh!

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

StanFromIreland added 2 commits July 14, 2025 10:29
@StanFromIreland
Clean up tests
3660160
@StanFromIreland
Remove unnecessary message
4e12b9e
@StanFromIreland
StanFromIreland marked this pull request as ready for review July 14, 2025 12:54
@bedevere-app bedevere-app Bot added the awaiting review label Jul 14, 2025
@StanFromIreland
StanFromIreland requested a review from picnixz July 14, 2025 12:54
ZeroIntensity
ZeroIntensity reviewed Jul 15, 2025
View reviewed changes

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

Would you mind running some microbenchmarks?

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Modules/_codecsmodule.c Resolved

Uh oh!

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

Comment thread Modules/_codecsmodule.c Resolved

Uh oh!

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

Comment thread Lib/encodings/__init__.py Resolved

Uh oh!

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

picnixz
picnixz reviewed Jul 15, 2025
View reviewed changes
Comment thread Lib/test/test_codecs.py Resolved

Uh oh!

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

Comment thread Lib/encodings/__init__.py Resolved

Uh oh!

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

Comment thread Lib/encodings/__init__.py Resolved

Uh oh!

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

@StanFromIreland
Review
1c9e55a
@StanFromIreland

StanFromIreland commented Jul 15, 2025 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

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
ZeroIntensity reacted with thumbs up emoji m-aciek reacted with rocket emoji
All reactions
  • 👍 1 reaction
  • 🚀 1 reaction

Sorry, something went wrong.

Uh oh!

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

ZeroIntensity
ZeroIntensity reviewed Jul 15, 2025
View reviewed changes
Comment thread Modules/_codecsmodule.c Resolved

Uh oh!

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

@malemburg

malemburg commented Jul 16, 2025

Copy link
Copy Markdown
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.

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

This was referenced Jul 16, 2025
Improve encodings.normalize_encoding behaviour or docs #136702
Open
gh-136736: Fix handling alphanumerical non-ASCII characters in encodings.normalize_encoding() #136737
Open
@serhiy-storchaka
serhiy-storchaka self-requested a review July 17, 2025 09:16
ZeroIntensity
ZeroIntensity reviewed Jul 18, 2025
View reviewed changes
Comment thread Modules/_codecsmodule.c Resolved

Uh oh!

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

@serhiy-storchaka

serhiy-storchaka commented Jul 21, 2025

Copy link
Copy Markdown
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).

All reactions

Sorry, something went wrong.

Uh oh!

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

@StanFromIreland

StanFromIreland commented Jul 21, 2025 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

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

@serhiy-storchaka

serhiy-storchaka commented Jul 21, 2025

Copy link
Copy Markdown
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.

All reactions

Sorry, something went wrong.

Uh oh!

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

malemburg
malemburg requested changes Jul 22, 2025
View reviewed changes
Comment thread Modules/_codecsmodule.c Resolved

Uh oh!

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

Comment thread Objects/unicodeobject.c Resolved

Uh oh!

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

@bedevere-app bedevere-app Bot added awaiting changes and removed awaiting review labels Jul 22, 2025
@bedevere-app

bedevere-app Bot commented Jul 22, 2025

Copy link
Copy Markdown

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.

All reactions

Sorry, something went wrong.

Uh oh!

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

@StanFromIreland
Changes
31c5107
@StanFromIreland

StanFromIreland commented Jul 27, 2025

Copy link
Copy Markdown
Member Author

I have made the requested changes; please review again

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-app bedevere-app Bot added awaiting change review and removed awaiting changes labels Jul 27, 2025
@bedevere-app

bedevere-app Bot commented Jul 27, 2025

Copy link
Copy Markdown

Thanks for making the requested changes!

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

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-app
bedevere-app Bot requested a review from malemburg July 27, 2025 19:17
@StanFromIreland

StanFromIreland commented Sep 6, 2025

Copy link
Copy Markdown
Member Author

Friendly ping @malemburg

All reactions

Sorry, something went wrong.

Uh oh!

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

@malemburg

malemburg commented Sep 9, 2025

Copy link
Copy Markdown
Member

I think we first need to have #137167 merged and then this one.

All reactions

Sorry, something went wrong.

Uh oh!

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

@malemburg malemburg mentioned this pull request Sep 9, 2025
gh-88886: Remove excessive encoding name normalization #137167
Merged
StanFromIreland and others added 2 commits October 2, 2025 16:17
@StanFromIreland
Merge branch 'main' into enc-normalize-speedup
8d50a7e
@StanFromIreland
!fixup merge
4912719
StanFromIreland
StanFromIreland commented Oct 2, 2025
View reviewed changes
Comment thread Python/codecs.c Resolved

Uh oh!

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

malemburg
malemburg approved these changes Oct 3, 2025
View reviewed changes

@malemburg malemburg 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. Please remove the unneeded forward def in codecs.c.

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Python/codecs.c Resolved

Uh oh!

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

@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting change review labels Oct 3, 2025
@StanFromIreland
Merge branch 'main' into enc-normalize-speedup
fea5b34
@StanFromIreland

StanFromIreland commented Oct 29, 2025

Copy link
Copy Markdown
Member Author

@malemburg Is there anything left to do here?

All reactions

Sorry, something went wrong.

Uh oh!

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

@malemburg
malemburg merged commit a3ce2f7 into python:main Oct 30, 2025
4 checks passed

Uh oh!

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

@bedevere-app bedevere-app Bot removed the awaiting merge label Oct 30, 2025
@malemburg

malemburg commented Oct 30, 2025

Copy link
Copy Markdown
Member

Thanks, @StanFromIreland, for working on this.

I don't think this can be backported, since it's a new feature in a way.

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

@StanFromIreland
StanFromIreland deleted the enc-normalize-speedup branch October 30, 2025 14:43
@StanFromIreland

StanFromIreland commented Oct 30, 2025

Copy link
Copy Markdown
Member Author

I agree.

All reactions

Sorry, something went wrong.

Uh oh!

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

StanFromIreland added a commit to StanFromIreland/cpython that referenced this pull request Dec 6, 2025
@StanFromIreland
pythongh-55531: Implement normalize_encoding in C (python#136643)
dfab25d
Closes pythongh-55531
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@picnixz picnixz picnixz left review comments
@ZeroIntensity ZeroIntensity ZeroIntensity left review comments
@malemburg malemburg malemburg approved these changes
@vstinner vstinner Awaiting requested review from vstinner
@serhiy-storchaka serhiy-storchaka Awaiting requested review from serhiy-storchaka

Uh oh!

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

Assignees

No one assigned

Labels

None yet

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

@StanFromIreland @malemburg @serhiy-storchaka @picnixz @ZeroIntensity
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.