Skip to content

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

Closed
vstinner wants to merge 1 commit into
python:masterfrom
vstinner:cp65001
Closed

bpo-36778: Avoid functools in encodings.cp65001#13110
vstinner wants to merge 1 commit into
python:masterfrom
vstinner:cp65001

Conversation

@vstinner

@vstinnervstinner commented May 6, 2019

Copy link
Copy Markdown
Member

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

Comment threadLib/encodings/cp65001.py Outdated

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

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

@paulmon

paulmon commented May 6, 2019

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Member

staticmethod is required, maybe.

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
Copy Markdown
MemberAuthor

staticmethod is required, maybe.

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

@vstinner

Copy link
Copy Markdown
MemberAuthor

@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

Copy link
Copy Markdown
MemberAuthor

I merged my PR #13230 instead.

@vstinner
vstinner deleted the cp65001 branch May 10, 2019 01:20
Sign up for freeto 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.

6 participants

@vstinner@paulmon@methane@serhiy-storchaka@the-knights-who-say-ni@bedevere-bot