bpo-31350: Optimize get_event_loop and _get_running_loop #3347

jimmylai · 2017-09-05T17:16:23Z

https://bugs.python.org/issue31350

…ure-close-loop [Asyncio] Call remove_done_callback in finally section to prevent orphan future cause RuntimeError: Event loop stopped before Future completed.

* 'master' of https://github.com/python/cpython: (601 commits) remove check for bug last seem in Solaris 9 (python#3285) Change code owners for hashlib and ssl to the crypto team (python#3284) bpo-31281: Fix pathlib.Path incompatibility in fileinput (pythongh-3208) remove autoconf check for select() (python#3283) remove configure check for 'volatile' (python#3281) Add missing _sha3 module to Setup.dist (python#2395) bpo-12383: Also ignore __PYVENV_LAUNCHER__ (python#3278) bpo-9146: add the missing NEWS entry. (python#3275) Fix a c.f.as_completed() refleak previously introduced in bpo-27144 (python#3270) bpo-31185: Fixed miscellaneous errors in asyncio speedup module. (python#3076) remove a redundant lower in urllib.parse.urlsplit (python#3008) bpo-31323: Fix reference leak in test_ssl (python#3263) bpo-31250, test_asyncio: fix EventLoopTestsMixin.tearDown() (python#3264) bpo-31326: ProcessPoolExecutor waits for the call queue thread (python#3265) bpo-27144: concurrent.futures as_complete and map iterators do not keep reference to returned object (python#1560) bpo-31250, test_asyncio: fix dangling threads (python#3252) bpo-31217: Fix regrtest -R for small integer (python#3260) bpo-30096: Use ABC in abc reference examples (python#1220) bpo-30737: Update DevGuide links to new URL (pythonGH-3228) [Trivial] Remove now redundant assert (python#3245) ...

* 'master' of https://github.com/python/cpython: (32 commits) Conceptually, roots is a set. Also searching it as a set is a tiny bit faster (python#3338) bpo-31343: Include sys/sysmacros.h (python#3318) bpo-30102: Call OPENSSL_add_all_algorithms_noconf (python#3112) Prevent a few make suspicious warnings. (python#3341) Include additional changes to support blurbified NEWS (python#3340) Simplify NEWS entry to prevent suspicious warnings. (python#3339) bpo-31347: _PyObject_FastCall_Prepend: do not call memcpy if args might not be null (python#3329) Revert "bpo-17852: Maintain a list of BufferedWriter objects. Flush them on exit. (python#1908)" (python#3337) bpo-17852: Maintain a list of BufferedWriter objects. Flush them on exit. (python#1908) Fix terminology in comment and add more design rationale. (python#3335) Add comment to explain the implications of not sorting keywords (python#3331) bpo-31170: Update libexpat from 2.2.3 to 2.2.4 (python#3315) bpo-28411: Remove "modules" field from Py_InterpreterState. (python#1638) random_triangular: sqrt() is more accurate than **0.5 (python#3317) Travis: use ccache (python#3307) remove IRIX support (closes bpo-31341) (python#3310) Code clean-up. Remove unnecessary pre-increment before the loop starts. (python#3312) Regen Moduls/clinic/_ssl.c.h (pythonGH-3320) bpo-30502: Fix handling of long oids in ssl. (python#2909) Cache externals, depending on changes to PCbuild (python#3308) ...

the-knights-who-say-ni · 2017-09-05T17:16:25Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

Thanks again to your contribution and we look forward to looking at it!

jimmylai · 2017-09-05T17:24:05Z

@1st1 The complete experiment result was posted in http://bugs.python.org/issue31350
This PR is expected to make get_event_loop() 17.71% faster

I'm sure I've signed CLA and also update bugs.python profile but don't know why it still not updated.

vstinner · 2017-09-05T17:28:24Z

Lib/asyncio/events.py

-
-_running_loop = _RunningLoop()
+_running_loop = threading.local()
+_running_loop._loop_pid = (None, None)


I don't think that the attribute has to be private, since the _running_loop variable is already prive. Rename _loop_pid to loop_pid.

@Haypo renamed.

vstinner · 2017-09-05T17:35:38Z

I'm now waiting for the benchmark code: https://bugs.python.org/issue31350#msg301343

jimmylai · 2017-09-05T17:41:23Z

@Haypo benchmark code was added. The code was reviewed by @1st1 yesterday afternoon.

1st1 · 2017-09-05T18:24:37Z

Lib/asyncio/events.py

-
-_running_loop = _RunningLoop()
+_running_loop = threading.local()
+_running_loop.loop_pid = (None, None)



This change needs to be reverted to:

class _RunningLoop(threading.local): _loop_pid = (None, None)

The problem is that _running_loop.loop_pid = (None, None) initializes _running_loop.loop_pid only for the current thread. All other threads using _running_loop.loop_pid will see None.

vstinner

LGTM.

serhiy-storchaka · 2017-09-05T19:07:29Z

Misc/NEWS.d/next/Library/2017-09-05-10-30-48.bpo-31350.dXJ-7N.rst

@@ -0,0 +1 @@
+Optimize get_event_loop and _get_running_loop to become 17.71% faster.


Don't use so much digits. In Victor's benchmark the speed up is only about 10%.

bedevere-bot · 2017-09-05T19:07:42Z

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 didn't expect the Spanish Inquisition!. 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.

vstinner · 2017-09-05T21:13:36Z

Misc/NEWS.d/next/Library/2017-09-05-10-30-48.bpo-31350.dXJ-7N.rst

@@ -0,0 +1 @@
+Micro-optimize _get_running_loop to become up to 10% faster.


Please replace _get_running_loop with:

:func:`asyncio.get_event_loop`

_get_running_loop is part of public API, btw :)

"_" marks private functions, no?

miss-islington · 2017-09-06T00:37:03Z

🐍🍒⛏🤖 Thanks @jimmylai for the PR, and @1st1 for merging it 🌮🎉.I'm working now to backport this PR to: 3.6.

…nGH-3347) * call remove_done_callback in finally section * Optimize get_event_loop and _get_running_loop * rename _loop_pid as loop_pid and add blurb news * rename _loop_pid as loop_pid and add blurb news * add back _RunningLoop * Update 2017-09-05-10-30-48.bpo-31350.dXJ-7N.rst * Update 2017-09-05-10-30-48.bpo-31350.dXJ-7N.rst (cherry picked from commit 80bbe6a)

bedevere-bot · 2017-09-06T00:37:10Z

GH-3373 is a backport of this pull request to the 3.6 branch.

…H-3373) * call remove_done_callback in finally section * Optimize get_event_loop and _get_running_loop * rename _loop_pid as loop_pid and add blurb news * rename _loop_pid as loop_pid and add blurb news * add back _RunningLoop * Update 2017-09-05-10-30-48.bpo-31350.dXJ-7N.rst * Update 2017-09-05-10-30-48.bpo-31350.dXJ-7N.rst (cherry picked from commit 80bbe6a)

* call remove_done_callback in finally section * Optimize get_event_loop and _get_running_loop * rename _loop_pid as loop_pid and add blurb news * rename _loop_pid as loop_pid and add blurb news * add back _RunningLoop * Update 2017-09-05-10-30-48.bpo-31350.dXJ-7N.rst * Update 2017-09-05-10-30-48.bpo-31350.dXJ-7N.rst

jimmylai and others added 5 commits May 20, 2017 18:40

call remove_done_callback in finally section

e5ec056

Merge pull request #1 from jimmylai/jimmylai-patch-asyncio-orphan-fut…

9f7ad5a

…ure-close-loop [Asyncio] Call remove_done_callback in finally section to prevent orphan future cause RuntimeError: Event loop stopped before Future completed.

Optimize get_event_loop and _get_running_loop

1036316

jimmylai requested a review from 1st1 as a code owner September 5, 2017 17:16

the-knights-who-say-ni added the CLA not signed label Sep 5, 2017

bedevere-bot added the awaiting review label Sep 5, 2017

jimmylai changed the title ~~Optimize get_event_loop and _get_running_loop~~ bpo-31350: Optimize get_event_loop and _get_running_loop Sep 5, 2017

vstinner reviewed Sep 5, 2017

View reviewed changes

Jimmy Lai added 2 commits September 5, 2017 10:32

rename _loop_pid as loop_pid and add blurb news

425ebb7

rename _loop_pid as loop_pid and add blurb news

443f6dd

bedevere-bot added awaiting core review and removed awaiting review labels Sep 5, 2017

1st1 reviewed Sep 5, 2017

View reviewed changes

add back _RunningLoop

920a244

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Sep 5, 2017

1st1 approved these changes Sep 5, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Sep 5, 2017

vstinner approved these changes Sep 5, 2017

View reviewed changes

serhiy-storchaka requested changes Sep 5, 2017

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting merge labels Sep 5, 2017

Update 2017-09-05-10-30-48.bpo-31350.dXJ-7N.rst

db5d743

vstinner requested changes Sep 5, 2017

View reviewed changes

Update 2017-09-05-10-30-48.bpo-31350.dXJ-7N.rst

e2e6fcb

1st1 added the needs backport to 3.6 label Sep 6, 2017

1st1 merged commit 80bbe6a into python:master Sep 6, 2017

bedevere-bot removed the needs backport to 3.6 label Sep 6, 2017

Mariatta removed the awaiting changes label Oct 4, 2017

		@@ -0,0 +1 @@
		Optimize get_event_loop and _get_running_loop to become 17.71% faster.

		@@ -0,0 +1 @@
		Micro-optimize _get_running_loop to become up to 10% faster.

Uh oh!

bpo-31350: Optimize get_event_loop and _get_running_loop #3347

bpo-31350: Optimize get_event_loop and _get_running_loop #3347

Uh oh!

Conversation

jimmylai commented Sep 5, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

the-knights-who-say-ni commented Sep 5, 2017

Uh oh!

jimmylai commented Sep 5, 2017

Uh oh!

vstinner Sep 5, 2017

Choose a reason for hiding this comment

Uh oh!

jimmylai Sep 5, 2017

Choose a reason for hiding this comment

Uh oh!

vstinner commented Sep 5, 2017

Uh oh!

jimmylai commented Sep 5, 2017

Uh oh!

1st1 Sep 5, 2017

Choose a reason for hiding this comment

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Sep 5, 2017

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Sep 5, 2017

Uh oh!

vstinner Sep 5, 2017

Choose a reason for hiding this comment

Uh oh!

1st1 Sep 5, 2017

Choose a reason for hiding this comment

Uh oh!

vstinner Sep 5, 2017

Choose a reason for hiding this comment

Uh oh!

miss-islington commented Sep 6, 2017

Uh oh!

bedevere-bot commented Sep 6, 2017

Uh oh!

Uh oh!

jimmylai commented Sep 5, 2017 •

edited by bedevere-bot

Loading