gh-103793: Defer formatting task name#103767

itamaro · 2023-04-24T16:30:10Z

The default task name is Task-<counter> (if no name is passed in during Task creation). This is initialized in Task.__init__ (C impl) using string formatting, which can be quite slow. Actually using the task name in real world code is not very common, so this is wasted init.

Let's defer this string formatting to the first time the name is read (in get_name impl), so we don't need to pay the string formatting cost if the task name is never read.

Fixesgh-103793

performance analysis

in a full run of pyperformance, this is 1.00x faster overall, and up to 5% faster on async benchmarks.

full report in this gist.

Modules/_asynciomodule.c

The default task name is "Task-<counter>" (if no name is passed in during Task creation). This is initialized in `Task.__init__` (C impl) using string formatting, which can be quite slow. Actually using the task name in real world code is not very common, so this is wasted init. Let's defer this string formatting to the first time the name is read (in `get_name` impl), so we don't need to pay the string formatting cost if the task name is never read.

jbower-fb · 2023-04-25T01:56:49Z

Strictly speaking, it's probably not necessary to store the task number in Task.__init__(). We could just call _task_name_counter() on lazy creation of a name. The only downside to this is tasks would end up being numbered slightly differently after this change for the same programs. However, I doubt anyone except possibly one or two easily fixed tests are relying on this. In general I imagine the number is actually non-deterministic.

Not storing the name on construction saves memory and will likely make everything a tiny bit faster.

…ng the next counter at lazy name generation

…on into defer-task-name-formatting

itamaro · 2023-04-25T04:40:22Z

We could just call _task_name_counter() on lazy creation of a name

that's clever! I like it :)

made this change to the PR. CI is still running, but at least on my local test run it didn't break any tests.

gvanrossum

Some thoughts.

gvanrossum · 2023-04-25T05:23:49Z

Lib/asyncio/tasks.py

-self._name=f'Task-{_task_name_counter()}'
+# optimization: defer task name formatting to first get_name
+self._name=None


Why do we need to optimize the .py version (which is normally not run since there is a C accelerator version)?

we definitely don't need to optimize the python version. I assumed we wanted to maintain parity between the versions, but maybe it doesn't matter to this extent (or my assumption was wrong and parity is not a goal).
I will revert the changes to the python version.

Modules/_asynciomodule.c

kumaraditya303 · 2023-04-25T10:09:06Z

Strictly speaking, it's probably not necessary to store the task number in Task.init(). We could just call _task_name_counter() on lazy creation of a name. The only downside to this is tasks would end up being numbered slightly differently after this change for the same programs. However, I doubt anyone except possibly one or two easily fixed tests are relying on this. In general I imagine the number is actually non-deterministic.

No, tasks numbers are assigned as they are allocated, its a little debugging aid and should not be changed.

kumaraditya303

See my suggestion on issue.

bedevere-bot · 2023-04-25T10:09:34Z

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.

itamaro · 2023-04-26T21:18:34Z

I have made the requested changes; please review again

Apologies about the review request spam - I don't know why GitHub decided to do that 🤔 (and I don't have permissions to clean up the reviewers list)

bedevere-bot · 2023-04-26T21:18:38Z

Thanks for making the requested changes!

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

JelleZijlstra

Looks good, nice speed win

gvanrossum

LGTM, but did you re-run the benchmark, now that we have a PyLong object creation?

And did you run the buildbots with the latest version to check for leaks?

We implemented something even simpler than a tagged pointer.

bedevere-bot · 2023-04-29T01:09:17Z

🤖 New build scheduled with the buildbot fleet by @gvanrossum for commit 592d44b 🤖

If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again.

gvanrossum · 2023-04-29T01:10:25Z

(Dismissing Kumar's review for him, he told me he's busy for the next few weeks. Setting the refleaks-buildbots label, those are sufficient for me.)

itamaro · 2023-04-29T17:19:18Z

I did rerun the benchmarks, it was 5-7% faster on the async benchmarks. I'll rerun again on main vs the parent commit and share the full report.

gvanrossum · 2023-04-29T17:46:28Z

Thanks! (And just because I work on weekends doesn’t mean I expect you to.)

itamaro · 2023-04-30T00:02:01Z

pyperformance async benchmarks 4-7% faster

3.12-base.20230429.1.json ========================= Performance version: 1.0.6 Python version: 3.12.0a7+ (64-bit) revision fbf3596c3e Report on Linux-5.15.0-1033-aws-x86_64-with-glibc2.31 Number of logical CPUs: 72 Start date: 2023-04-29 16:39:30.287074 End date: 2023-04-29 16:50:17.995978 3.12-defer.20230429.1.json ========================== Performance version: 1.0.6 Python version: 3.12.0a7+ (64-bit) revision 85c7bf5bce Report on Linux-5.15.0-1033-aws-x86_64-with-glibc2.31 Number of logical CPUs: 72 Start date: 2023-04-29 16:12:36.207389 End date: 2023-04-29 16:22:51.562679 +-------------------------------+---------------------------+----------------------------+--------------+-----------------------+ | Benchmark | 3.12-base.20230429.1.json | 3.12-defer.20230429.1.json | Change | Significance | +===============================+===========================+============================+==============+=======================+ | async_tree_cpu_io_mixed | 867 ms | 835 ms | 1.04x faster | Significant (t=6.62) | +-------------------------------+---------------------------+----------------------------+--------------+-----------------------+ | async_tree_io | 1.47 sec | 1.39 sec | 1.06x faster | Significant (t=21.03) | +-------------------------------+---------------------------+----------------------------+--------------+-----------------------+ | async_tree_memoization | 735 ms | 696 ms | 1.06x faster | Significant (t=21.10) | +-------------------------------+---------------------------+----------------------------+--------------+-----------------------+ | async_tree_none | 611 ms | 570 ms | 1.07x faster | Significant (t=13.44) | +-------------------------------+---------------------------+----------------------------+--------------+-----------------------+

microbenchmark ~10% faster

python -m pyperf timeit -s 'import asyncio' -s 'async def coro(): pass' -s 'async def make_tasks(): return [asyncio.Task(coro()) for _ in range(10000)]' 'asyncio.run(make_tasks())'

with this PR:

Mean +- std dev: 27.5 ms +- 0.6 ms

on parent commit:

Mean +- std dev: 30.5 ms +- 0.9 ms

gvanrossum · 2023-04-30T00:08:52Z

Excellent—thanks!

* main: (26 commits) pythongh-104028: Reduce object creation while calling callback function from gc (pythongh-104030) pythongh-104036: Fix direct invocation of test_typing (python#104037) pythongh-102213: Optimize the performance of `__getattr__` (pythonGH-103761) pythongh-103895: Improve how invalid `Exception.__notes__` are displayed (python#103897) Adjust expression from `==` to `!=` in alignment with the meaning of the paragraph. (pythonGH-104021) pythongh-88496: Fix IDLE test hang on macOS (python#104025) Improve int test coverage (python#104024) pythongh-88773: Added teleport method to Turtle library (python#103974) pythongh-104015: Fix direct invocation of `test_dataclasses` (python#104017) pythongh-104012: Ensure test_calendar.CalendarTestCase.test_deprecation_warning consistently passes (python#104014) pythongh-103977: compile re expressions in platform.py only if required (python#103981) pythongh-98003: Inline call frames for CALL_FUNCTION_EX (pythonGH-98004) Replace Netlify with Read the Docs build previews (python#103843) Update name in acknowledgements and add mailmap (python#103696) pythongh-82054: allow test runner to split test_asyncio to execute in parallel by sharding. (python#103927) Remove non-existing tools from Sundry skiplist (python#103991) pythongh-103793: Defer formatting task name (python#103767) pythongh-87092: change assembler to use instruction sequence instead of CFG (python#103933) pythongh-103636: issue warning for deprecated calendar constants (python#103833) Various small fixes to dis docs (python#103923) ...

bedevere-bot added the awaiting review label Apr 24, 2023

itamaro changed the title ~~gh-NNNNN: Defer formatting task name~~gh-103793: Defer formatting task nameApr 24, 2023

bedevere-bot mentioned this pull request Apr 24, 2023
Defer string formatting in asyncio Task creation #103793
Closed

gaogaotiantian reviewed Apr 24, 2023
View reviewed changes

Modules/_asynciomodule.c Outdated Show resolvedHide resolved

itamaro added 2 commits April 24, 2023 16:32

Defer task name formatting also in Python Task impl
1822ff8

itamaro force-pushed the defer-task-name-formatting branch from db3b8a6 to dbfe832Compare April 24, 2023 22:32

Add NEWS and whatsnew entries
74d0084

itamaro force-pushed the defer-task-name-formatting branch from dbfe832 to 74d0084Compare April 24, 2023 22:58

itamaro marked this pull request as ready for review April 24, 2023 23:24

itamaro requested review from 1st1, asvetlov, gvanrossum and kumaraditya303 as code owners April 24, 2023 23:24

Merge branch 'main' into defer-task-name-formatting
dbc7047

itamaro added 4 commits April 24, 2023 21:14

Merge branch 'main' into defer-task-name-formatting
2bb6c1e

replace storing the task counter on task construction with just getti…
2efd376
…ng the next counter at lazy name generation

Merge branch 'defer-task-name-formatting' of github.com:itamaro/cpyth…
6cb321b
…on into defer-task-name-formatting

remove redundant return
7cde290

gvanrossum reviewed Apr 25, 2023
View reviewed changes

kumaraditya303 previously requested changes Apr 25, 2023
View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Apr 25, 2023

itamaro added 3 commits April 25, 2023 09:02

Merge branch 'main' into defer-task-name-formatting
4b50646

revert changes to python version
a155190

pass error up if PyUnicode_FromFormat returns NULL
fb2bd7f

erlend-aasland removed request for berkerpeksag and erlend-aasland April 26, 2023 21:00

bedevere-bot added awaiting change review and removed awaiting changes labels Apr 26, 2023

bedevere-bot requested a review from kumaraditya303 April 26, 2023 21:18

hugovk removed request for ezio-melotti, hugovk and willingc April 27, 2023 00:39

willingc self-requested a review April 27, 2023 15:25

itamaro requested a review from gvanrossum April 28, 2023 21:57

JelleZijlstra approved these changes Apr 29, 2023
View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Apr 29, 2023

gvanrossum approved these changes Apr 29, 2023
View reviewed changes

gvanrossum added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Apr 29, 2023

bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Apr 29, 2023

gvanrossum merged commit 85c7bf5 into python:mainApr 29, 2023

bedevere-bot removed the awaiting merge label Apr 29, 2023

itamaro deleted the defer-task-name-formatting branch April 30, 2023 01:02

gvanrossum mentioned this pull request May 5, 2023
gh-104144: Skip scheduling a done callback if a TaskGroup task completes eagerly #104140
Merged

Uh oh!

gh-103793: Defer formatting task name#103767

gh-103793: Defer formatting task name #103767

Uh oh!

Conversation

itamaro commented Apr 24, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

performance analysis

Uh oh!

Uh oh!

jbower-fb commented Apr 25, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

itamaro commented Apr 25, 2023

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

gvanrossumApr 25, 2023

Choose a reason for hiding this comment

Uh oh!

itamaroApr 25, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kumaraditya303 commented Apr 25, 2023

Uh oh!

kumaraditya303 left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Apr 25, 2023

Uh oh!

itamaro commented Apr 26, 2023

Uh oh!

bedevere-bot commented Apr 26, 2023

Uh oh!

JelleZijlstra left a comment

Choose a reason for hiding this comment

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Apr 29, 2023

Uh oh!

gvanrossum commented Apr 29, 2023

Uh oh!

itamaro commented Apr 29, 2023

Uh oh!

gvanrossum commented Apr 29, 2023

Uh oh!

itamaro commented Apr 30, 2023

pyperformance async benchmarks 4-7% faster

microbenchmark ~10% faster

Uh oh!

gvanrossum commented Apr 30, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

itamaro commented Apr 24, 2023•
edited
Loading

jbower-fb commented Apr 25, 2023•
edited
Loading