Simplify MAKE_FUNCTION

[serprex]

BPO	27095
Nosy	@markshannon, @serhiy-storchaka, @serprex
Files	mkfu.patch mkfu2.patch mkfu3.patch: wordcode mkfu4.patch mkfu5.patch mkfu5.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/serhiy-storchaka'
closed_at = <Date 2016-06-12.15:13:20.823>
created_at = <Date 2016-05-23.21:26:20.787>
labels = ['interpreter-core', 'performance']
title = 'Simplify MAKE_FUNCTION'
updated_at = <Date 2016-06-12.15:13:20.821>
user = 'https://github.com/serprex'

bugs.python.org fields:

activity = <Date 2016-06-12.15:13:20.821>
actor = 'serhiy.storchaka'
assignee = 'serhiy.storchaka'
closed = True
closed_date = <Date 2016-06-12.15:13:20.823>
closer = 'serhiy.storchaka'
components = ['Interpreter Core']
creation = <Date 2016-05-23.21:26:20.787>
creator = 'Demur Rumed'
dependencies = []
files = ['42959', '42960', '42965', '43353', '43362', '43363']
hgrepos = []
issue_num = 27095
keywords = ['patch']
message_count = 13.0
messages = ['266187', '266205', '266230', '266245', '266512', '268283', '268303', '268324', '268373', '268375', '268377', '268388', '268389']
nosy_count = 4.0
nosy_names = ['Mark.Shannon', 'python-dev', 'serhiy.storchaka', 'Demur Rumed']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'performance'
url = 'https://bugs.python.org/issue27095'
versions = ['Python 3.6']

[serprex]

Implemented as per https://mail.python.org/pipermail/python-dev/2016-April/144135.html

[serprex]

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

[serhiy-storchaka]

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.

[serprex]

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

[serhiy-storchaka]

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 (bpo-27140) new code would be only one instruction longer.

[serhiy-storchaka]

Now with adding BUILD_CONST_KEY_MAP I think MAKE_FUNCTION can be more compact.

[serprex]

mkfu4 implements bpo-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

[serhiy-storchaka]

Added more comments on Rietveld.

[serprex]

Kind of amusing how visit_argannoation logic went full circle. Makes sense considering pre-mkfu patch the ABI was quite similar on that front

[serhiy-storchaka]

Yes, this is why I wanted first push the patch with BUILD_CONST_KEY_MAP.

[serhiy-storchaka]

Regenerated for review.

[python-dev]

New changeset 8a0fe3481c91 by Serhiy Storchaka in branch 'default':
Issue bpo-27095: Simplified MAKE_FUNCTION and removed MAKE_CLOSURE opcodes.
https://hg.python.org/cpython/rev/8a0fe3481c91

[serhiy-storchaka]

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 bpo-26647.

After pushing I found a bug (similar bug was existing) in compile.c. I'll open separate issue for this.