Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify MAKE_FUNCTION #71282

Closed
serprex mannequin opened this issue May 23, 2016 · 13 comments
Closed

Simplify MAKE_FUNCTION #71282

serprex mannequin opened this issue May 23, 2016 · 13 comments
Assignees
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage

Comments

@serprex
Copy link
Mannequin

serprex mannequin commented May 23, 2016

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
    Copy link
    Mannequin Author

    serprex mannequin commented May 23, 2016

    @serprex serprex mannequin added the performance Performance or resource usage label May 23, 2016
    @serhiy-storchaka serhiy-storchaka added the interpreter-core (Objects, Python, Grammar, and Parser dirs) label May 23, 2016
    @serprex
    Copy link
    Mannequin Author

    serprex mannequin commented 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
    Copy link
    Member

    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
    Copy link
    Mannequin Author

    serprex mannequin commented 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
    Copy link
    Member

    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
    Copy link
    Member

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

    @serprex
    Copy link
    Mannequin Author

    serprex mannequin commented Jun 12, 2016

    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
    Copy link
    Member

    Added more comments on Rietveld.

    @serprex
    Copy link
    Mannequin Author

    serprex mannequin commented 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
    Copy link
    Member

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

    @serhiy-storchaka
    Copy link
    Member

    Regenerated for review.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented 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
    Copy link
    Member

    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.

    @serhiy-storchaka serhiy-storchaka self-assigned this Jun 12, 2016
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage
    Projects
    None yet
    Development

    No branches or pull requests

    1 participant