classification
Title: Simplify MAKE_FUNCTION
Type: performance Stage: resolved
Components: Interpreter Core Versions: Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: Demur Rumed, Mark.Shannon, python-dev, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2016-05-23 21:26 by Demur Rumed, last changed 2016-06-12 15:13 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
mkfu.patch Demur Rumed, 2016-05-23 21:26 review
mkfu2.patch Demur Rumed, 2016-05-23 23:18 review
mkfu3.patch Demur Rumed, 2016-05-24 12:27 wordcode review
mkfu4.patch Demur Rumed, 2016-06-12 00:47 review
mkfu5.patch Demur Rumed, 2016-06-12 13:13
mkfu5.patch serhiy.storchaka, 2016-06-12 13:28 review
Messages (13)
msg266187 - (view) Author: Demur Rumed (Demur Rumed) * Date: 2016-05-23 21:26
Implemented as per https://mail.python.org/pipermail/python-dev/2016-April/144135.html
msg266205 - (view) Author: Demur Rumed (Demur Rumed) * Date: 2016-05-23 23:18
Attached are modifications to the patch based on feedback from Nikita. It produces a larger patch tho. The changes are to combine return branches in compiler_make_closure & to combine code between compiler_function & compiler_lambda into compiler_default_arguments
msg266230 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-05-24 05:51
At first look your patch looks good (I'll make more detailed review later), but you should wait until we have finished all work related to the conversion to wordcode.
msg266245 - (view) Author: Demur Rumed (Demur Rumed) * Date: 2016-05-24 12:27
May've been best to wait on posting a patch, but long weekend yesterday made time available

mkfu3 updates mkfu2 with wordcode. Includes fix to EXTENDED_ARG documentation
msg266512 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-05-27 20:44
In general the patch LGTM, besides few minor comments.

But new implementation uses longer bytecode for annotations. Current code packs argument names in one constant tuple:

$ ./python -m dis
def f(x: int, y: str, z: float): pass
  1           0 LOAD_NAME                0 (int)
              2 LOAD_NAME                1 (str)
              4 LOAD_NAME                2 (float)
              6 LOAD_CONST               0 (('x', 'y', 'z'))
              8 LOAD_CONST               1 (<code object f at 0xb6fc1640, file "<stdin>", line 1>)
             10 LOAD_CONST               2 ('f')
             12 EXTENDED_ARG             4
             14 EXTENDED_ARG          1024
             16 MAKE_FUNCTION        262144
             18 STORE_NAME               3 (f)
             20 LOAD_CONST               3 (None)
             22 RETURN_VALUE

New code uses LOAD_CONST for every name:

$ ./python -m dis
def f(x: int, y: str, z: float): pass
  1           0 LOAD_CONST               0 ('x')
              2 LOAD_NAME                0 (int)
              4 LOAD_CONST               1 ('y')
              6 LOAD_NAME                1 (str)
              8 LOAD_CONST               2 ('z')
             10 LOAD_NAME                2 (float)
             12 BUILD_MAP                3
             14 LOAD_CONST               3 (<code object f at 0xb7035a20, file "<stdin>", line 1>)
             16 LOAD_CONST               4 ('f')
             18 MAKE_FUNCTION            4
             20 STORE_NAME               3 (f)
             22 LOAD_CONST               5 (None)
             24 RETURN_VALUE

With new opcode that creates a dict from values and a tuple of keys (issue27140) new code would be only one instruction longer.
msg268283 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-06-11 21:46
Now with adding BUILD_CONST_KEY_MAP I think MAKE_FUNCTION can be more compact.
msg268303 - (view) Author: Demur Rumed (Demur Rumed) * Date: 2016-06-12 00:47
mkfu4 implements #27140. It doesn't special case 1-tuples into `BUILD_MAP 1`

It may be easier to have `BUILD_CONST_KEY_MAP 1` peepholed if it's really preferable to strength reduce

I'm also noticing that it could've been suggested to go to the extreme with BUILD_CONST_KEY_MAP where instead of relying on peepholer to constant fold tuple, we implement constant folding const keys entirely there. Just a random thought, not a suggestion or anything else
msg268324 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-06-12 04:56
Added more comments on Rietveld.
msg268373 - (view) Author: Demur Rumed (Demur Rumed) * Date: 2016-06-12 13:13
Kind of amusing how visit_argannoation logic went full circle. Makes sense considering pre-mkfu patch the ABI was quite similar on that front
msg268375 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-06-12 13:18
Yes, this is why I wanted first push the patch with BUILD_CONST_KEY_MAP.
msg268377 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-06-12 13:28
Regenerated for review.
msg268388 - (view) Author: Roundup Robot (python-dev) Date: 2016-06-12 14:37
New changeset 8a0fe3481c91 by Serhiy Storchaka in branch 'default':
Issue #27095: Simplified MAKE_FUNCTION and removed MAKE_CLOSURE opcodes.
https://hg.python.org/cpython/rev/8a0fe3481c91
msg268389 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-06-12 15:13
Pushed with minor changes. Thank you for your contribution Demur!

You change to the documentation of EXTENDED_ARG isn't pushed. I would prefer to see it as a part of larger documentation patch in issue26647.

After pushing I found a bug (similar bug was existing) in compile.c. I'll open separate issue for this.
History
Date User Action Args
2016-06-12 15:13:20serhiy.storchakasetstatus: open -> closed
messages: + msg268389

assignee: serhiy.storchaka
dependencies: - ceval: use Wordcode, 16-bit bytecode
resolution: fixed
stage: patch review -> resolved
2016-06-12 14:37:18python-devsetnosy: + python-dev
messages: + msg268388
2016-06-12 13:28:50serhiy.storchakasetfiles: + mkfu5.patch

messages: + msg268377
2016-06-12 13:18:01serhiy.storchakasetmessages: + msg268375
2016-06-12 13:13:34Demur Rumedsetfiles: + mkfu5.patch

messages: + msg268373
2016-06-12 04:56:34serhiy.storchakasetmessages: + msg268324
2016-06-12 00:47:45Demur Rumedsetfiles: + mkfu4.patch

messages: + msg268303
2016-06-11 21:46:34serhiy.storchakasetmessages: + msg268283
2016-06-04 21:06:05serhiy.storchakasetnosy: + Mark.Shannon
2016-05-27 20:44:22serhiy.storchakasetmessages: + msg266512
stage: patch review
2016-05-24 12:27:25Demur Rumedsetfiles: + mkfu3.patch

messages: + msg266245
2016-05-24 05:51:59serhiy.storchakasetmessages: + msg266230
2016-05-23 23:18:49Demur Rumedsetfiles: + mkfu2.patch

messages: + msg266205
2016-05-23 21:42:47serhiy.storchakasetnosy: + serhiy.storchaka
dependencies: + ceval: use Wordcode, 16-bit bytecode
components: + Interpreter Core
2016-05-23 21:26:28Demur Rumedsetversions: + Python 3.6
2016-05-23 21:26:20Demur Rumedcreate