Skip to content

bpo-36778: Avoid functools in encodings.cp65001 #13110

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

bpo-36778: Avoid functools in encodings.cp65001 #13110

wants to merge 1 commit into from

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented May 6, 2019 •
edited by bedevere-bot
Loading

Replace functools.partial() with lambda to reduce the number of
imports at startup. Avoid the following imports at startup:

  • _collections
  • _functools
  • _heapq
  • _operator
  • collections
  • functools
  • heapq
  • itertools
  • keyword
  • operator
  • reprlib

https://bugs.python.org/issue36778

@vstinner vstinner requested a review from methane May 6, 2019 15:04
serhiy-storchaka
serhiy-storchaka reviewed May 6, 2019
View reviewed changes
Lib/encodings/cp65001.py

if not hasattr(codecs, 'code_page_encode'):
raise LookupError("cp65001 encoding is only available on Windows")

### Codec APIs

encode = functools.partial(codecs.code_page_encode, 65001)
_decode = functools.partial(codecs.code_page_decode, 65001)
encode = lambda *args, **kw: codecs.code_page_encode(65001, *args, **kw)
Copy link
Member

Choose a reason for hiding this comment

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

Could you please measure how much overhead it adds?

@vstinner
Copy link
Member Author

vstinner commented May 6, 2019

Oh, my PR looks wrong. Most CP65001Test tests fail...

@paulmon
Copy link
Contributor

paulmon commented May 6, 2019 •
edited
Loading

Oh, my PR looks wrong. Most CP65001Test tests fail...

I don't know if it helps, but test_startup_imports passes on IoT Core using this change with a default codepage of cp65001. So it looks like this strategy could work if there's a way to remove functools without breaking other things.

@methane
Copy link
Member

methane commented May 7, 2019

staticmethod is required, maybe.

@paulmon paulmon mentioned this pull request May 8, 2019
Closed
@vstinner
bpo-36778: Avoid functools in encodings.cp65001
f26d4c5 
Replace functools.partial() with lambda to reduce the number of
imports at startup. Avoid the following imports at startup:

- _collections
- _functools
- _heapq
- _operator
- collections
- functools
- heapq
- itertools
- keyword
- operator
- reprlib
@vstinner
Copy link
Member Author

vstinner commented May 9, 2019

staticmethod is required, maybe.

Right. I fixed my PR with that. I also rebased my PR and added a NEWS entry.

@vstinner
Copy link
Member Author

vstinner commented May 9, 2019

@serhiy-storchaka: "Could you please measure how much overhead it adds?"

Benchmark in release mode:

>python -m venv env
>env\scripts\python_d.exe -m pip install perf
>env\scripts\python_d.exe -m perf timeit -s "import codecs; encode=codecs.lookup('cp65001').encode" "encode('')" -v -o ref.json
>git co master
>env\scripts\python_d.exe -m perf timeit -s "import codecs; encode=codecs.lookup('cp65001').encode" "encode('')" -v -o patch.json
env\scripts\python_d.exe -m perf compare_to ref.json patch.json

Mean +- std dev: [ref] 156 ns +- 3 ns -> [patch] 417 ns +- 17 ns: 2.68x slower (+168%)

@vstinner vstinner mentioned this pull request May 9, 2019
Merged
@vstinner
Copy link
Member Author

I merged my PR #13230 instead.

@vstinner vstinner closed this May 10, 2019
@vstinner vstinner deleted the cp65001 branch May 10, 2019 01:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka left review comments

@methane methane Awaiting requested review from methane

Assignees
No one assigned
Labels
awaiting core review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@vstinner @paulmon @methane @serhiy-storchaka @the-knights-who-say-ni @bedevere-bot