classification
Title: Speedup types.coroutine()
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.6, Python 3.5
process
Status: closed Resolution: rejected
Dependencies: 24400 Superseder:
Assigned To: yselivanov Nosy List: Arfrever, Mark.Shannon, asvetlov, gvanrossum, larry, martin.panter, ncoghlan, python-dev, scoder, vstinner, yselivanov
Priority: normal Keywords: patch

Created on 2015-05-29 16:23 by yselivanov, last changed 2018-01-29 20:10 by yselivanov. This issue is now closed.

Files
File name Uploaded Description Edit
types_coroutine_speedup.patch yselivanov, 2015-05-29 16:23 review
types_coroutine_speedup.patch yselivanov, 2015-05-29 21:10 review
types_coroutine_speedup.patch yselivanov, 2015-06-20 21:37 more tests; rebase onto 24400; make GeneratorWrapper a GC type
types_coroutine_speedup.patch yselivanov, 2015-06-22 22:09 rebased; more tests review
types_coroutine_speedup.patch yselivanov, 2015-06-24 17:01 new unittests have been committed; the patch now contains only the acceleration part review
Messages (20)
msg244388 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-05-29 16:23
Attached patch provides an implementation (part of it) of types.coroutine in C.

The problem with the current pure Python implementation is that it copies the code object of the generator function, which is a small overhead during import.

I'm not sure if this should be merged in 3.5 at all.  Please take a look at the patch.
msg244418 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-05-29 21:10
Attached is the second iteration of the patch.  Now, besides just speeding up types.coroutine() for pure python generator functions, it also provides a better wrapper around generator-like objects.
msg244447 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2015-05-30 06:09
This looks big and complicated.  I'd prefer this skipped 3.5 and just went into 3.6.
msg244451 - (view) Author: Stefan Behnel (scoder) * (Python committer) Date: 2015-05-30 07:23
I would still like to see a patch in Py3.5 that only flips the bit in C when "_types" is available and otherwise falls back to the existing Python code that creates a new code object. This part is much cleaner and faster to do in C than in the current Python implementation.
msg244490 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-05-30 20:22
Larry, can you accept the first version of the patch (only function that patches code object's co_flags) after beta2?

I'm OK if you think it should only be committed in 3.6, but I also agree with Stefan, that using C is better in this particular case.
msg244504 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2015-05-31 00:35
Since it's a speedup it could also go into 3.5.1.
On May 30, 2015 1:22 PM, "Yury Selivanov" <report@bugs.python.org> wrote:

>
> Yury Selivanov added the comment:
>
> Larry, can you accept the first version of the patch (only function that
> patches code object's co_flags) after beta2?
>
> I'm OK if you think it should only be committed in 3.6, but I also agree
> with Stefan, that using C is better in this particular case.
>
> ----------
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue24325>
> _______________________________________
>
msg245575 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-06-20 21:37
Larry, Nick,

Looking at how issue24400 progresses, I think it would be really great if we can merge this one in 3.5.0.  It also makes it unnecessary to merge issue24468 in 3.5.
msg245581 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2015-06-21 01:59
+1 from me for merging this for 3.5.0 and deferring issue 24468 (which now proposes making _opcode a builtin module to allow compiler constants to be easily shared between C code and Python code) to 3.6 instead.

The design changes to address issue 24400 cleaned up various aspect of both the internal architecture and the public data model of PEP 492, and the latest draft of this patch benefits accordingly.
msg245655 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-06-22 22:09
A rebased version of the patch is attached (now a "review" link should appear).

Nick, Stefan, please take a look.

Larry, can we merge this in 3.5.0?  I've invested a lot of time to have 100% test coverage; the test suite is very elaborate now.  There seems to be no refleaks too.  I think this change is a very natural continuation of what we've done in issue24400.
msg245756 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-06-24 15:45
New changeset eb6fb8e2f995 by Yury Selivanov in branch '3.5':
Issue #24325, #24400: Add more unittests for types.coroutine; tweak wrapper implementation.
https://hg.python.org/cpython/rev/eb6fb8e2f995

New changeset 7a2a79362bbe by Yury Selivanov in branch 'default':
Merge 3.5 (Issue #24325, #24400)
https://hg.python.org/cpython/rev/7a2a79362bbe
msg245761 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-06-24 16:52
New changeset 9aee273bf8b7 by Yury Selivanov in branch '3.5':
Issue #24400, #24325: More tests for types._GeneratorWrapper
https://hg.python.org/cpython/rev/9aee273bf8b7

New changeset fa097a336079 by Yury Selivanov in branch 'default':
Merge 3.5 (issue #24325 & #24400)
https://hg.python.org/cpython/rev/fa097a336079
msg245764 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-06-24 17:01
I've committed new unittests from this patch (as they are applicable to pure Python implementation of the wrapper too)

The patch now contains only the C implementation of the wrapper and should be ready to be committed.  Larry?
msg246117 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2015-07-03 01:01
Help me to understand here.  You want to check in a patch adding 300 new lines of C code to the types module during beta, for a speed optimization, after we've already hit beta?

While I like speedups as much as the next guy, I would be happier if this waited for 3.6.

Of course, if Guido is overruling me, then Guido is overruling me and you get to check it in.
msg246118 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2015-07-03 01:25
Oh, wait, I was confusing myself.  This is that new module you guys created for type hints, and this is a new object with no installed base.  (Right?)

Yeah, you can check it in for 3.5.
msg246123 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-07-03 03:56
> Oh, wait, I was confusing myself.  This is that new module you guys created for type hints, and this is a new object with no installed base.  (Right?)

No, you were right in your previous comment...

> Help me to understand here.  You want to check in a patch adding 300 new lines of C code to the types module during beta, for a speed optimization, after we've already hit beta?

> While I like speedups as much as the next guy, I would be happier if this waited for 3.6.

This speedup will mostly affect code compiled with Cython. See the following example:
  
  @asyncio.coroutine
  def coro():
     yield from ...

Cython will compile "coro" into a function, that returns a generator-like object.  "asyncio.coroutine" will wrap this function, and, therefore, the optimized by Cython generator-like object will be wrapped too (to provide an __await__ method).

This patch provides a faster wrapper for such generator-like objects. Since the whole point of using Cython is to squeeze as much performance as possible, I think it's essential to have this optimization in 3.5 (or at least in 3.5.1 as Guido suggested).

It's a lot of C code, I agree. I only can say that I did my best to write very extensive unittests, and so I hope that it won't cause any trouble.
msg246139 - (view) Author: Stefan Behnel (scoder) * (Python committer) Date: 2015-07-03 06:23
This is not purely about speeding up the code. It's also about avoiding to replace the code object of a function, which is essentially a big and clumsy hack only to achieve setting a flag. Some tools, namely line_profiler, use the current code object as a dict key for state keeping. Replacing the reference in "__code__" might confuse them if they happened to catch a reference before.

That's why I asked for applying at least the simple patch that sets the flag with a C level helper function. But I'd be ok with applying the latest patch as it is. The non-flag-setting parts are simple and a straight forward translation of the current Python code.
msg246142 - (view) Author: Mark Shannon (Mark.Shannon) * (Python committer) Date: 2015-07-03 07:26
Does this have a measurable performance impact?
I'd be surprised if it did.

W.r.t. to profiling, the undecorated form will never be visible to any code other than the decorator, so won't show up in the profiler.
msg246148 - (view) Author: Stefan Behnel (scoder) * (Python committer) Date: 2015-07-03 07:47
> the undecorated form will never be visible to any code other than the decorator

Assuming that 1) it's the first and/or only decorator, 2) it's used to
decorate a generator function that returns its own generator and 3) it's
really used as a decorator and not as a general helper function to safely
wrap some object that was created somewhere else.
msg246330 - (view) Author: Mark Shannon (Mark.Shannon) * (Python committer) Date: 2015-07-05 18:41
If type.coroutine is not the first and only decorator, then things may be even worse.

Code objects are currently immutable.
This change would mean that a call to types.coroutine in one place in the code would change the semantics of another piece of code in a potentially surprising way.

I think creating a copy is probably the best thing to.
msg311162 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2018-01-29 20:10
Closing this one now--there's no point in speeding up types.coroutine anymore.
History
Date User Action Args
2018-01-29 20:10:43yselivanovsetstatus: open -> closed
resolution: rejected
messages: + msg311162

stage: patch review -> resolved
2015-07-05 18:41:42Mark.Shannonsetmessages: + msg246330
2015-07-03 07:47:30scodersetmessages: + msg246148
2015-07-03 07:26:50Mark.Shannonsetnosy: + Mark.Shannon
messages: + msg246142
2015-07-03 06:23:22scodersetmessages: + msg246139
2015-07-03 03:56:34yselivanovsetmessages: + msg246123
2015-07-03 01:25:08larrysetmessages: + msg246118
2015-07-03 01:01:09larrysetmessages: + msg246117
2015-06-24 17:01:26yselivanovsetfiles: + types_coroutine_speedup.patch

messages: + msg245764
2015-06-24 16:52:04python-devsetmessages: + msg245761
2015-06-24 15:45:33python-devsetnosy: + python-dev
messages: + msg245756
2015-06-22 22:12:52yselivanovsetnosy: + vstinner, asvetlov, martin.panter
2015-06-22 22:09:36yselivanovsetfiles: + types_coroutine_speedup.patch

messages: + msg245655
2015-06-21 01:59:05ncoghlansetmessages: + msg245581
2015-06-20 21:37:51yselivanovsetfiles: + types_coroutine_speedup.patch

dependencies: + Awaitable ABC incompatible with functools.singledispatch
messages: + msg245575
2015-06-07 13:26:41Arfreversetnosy: + Arfrever
2015-05-31 00:35:31gvanrossumsetmessages: + msg244504
2015-05-30 20:22:26yselivanovsetmessages: + msg244490
2015-05-30 07:23:56scodersetmessages: + msg244451
2015-05-30 06:09:02larrysetmessages: + msg244447
2015-05-30 05:47:00serhiy.storchakasetnosy: + larry
type: enhancement
2015-05-29 21:10:37yselivanovsetfiles: + types_coroutine_speedup.patch
nosy: + scoder
messages: + msg244418

2015-05-29 16:23:27yselivanovcreate