Repository navigation
Simplify MAKE_FUNCTION #71282
Description
| BPO | 27095 |
|---|---|
| Nosy | @markshannon, @serhiy-storchaka, @serprex |
| Files |
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']
Activity
serprex commentedon May 23, 2016
Implemented as per https://mail.python.org/pipermail/python-dev/2016-April/144135.html
serprex commentedon May 23, 2016
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 commentedon May 24, 2016
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 commentedon May 24, 2016
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 commentedon May 27, 2016
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_VALUENew code uses LOAD_CONST for every name:
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 commentedon Jun 11, 2016
Now with adding BUILD_CONST_KEY_MAP I think MAKE_FUNCTION can be more compact.
serprex commentedon Jun 12, 2016
mkfu4 implements bpo-27140. It doesn't special case 1-tuples into
BUILD_MAP 1It may be easier to have
BUILD_CONST_KEY_MAP 1peepholed if it's really preferable to strength reduceI'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 commentedon Jun 12, 2016
Added more comments on Rietveld.
serprex commentedon Jun 12, 2016
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 commentedon Jun 12, 2016
Yes, this is why I wanted first push the patch with BUILD_CONST_KEY_MAP.
serhiy-storchaka commentedon Jun 12, 2016
Regenerated for review.
python-dev commentedon Jun 12, 2016
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 commentedon Jun 12, 2016
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.