Description

markshannon

opened on Apr 1, 2021

Issue body actions

BPO	43693
Nosy	@gvanrossum, @rhettinger, @ncoghlan, @markshannon, @ericsnowcurrently, @serhiy-storchaka, @gvanrossum, @pablogsal
PRs	bpo-43693: Compute deref offsets in compiler #25152 bpo-43693: Group the code in codeobject.c logically. #26216 bpo-43693: Add _PyCode_New() and do some related cleanup. #26258 bpo-43693: Clean up the PyCodeObject fields. #26364 bpo-43693: Add _PyCode_New(). #26375 bpo-43693: Add new internal code objects fields: co_fastlocalnames and co_fastlocalkinds. #26388 bpo-43693: Add the MAKE_CELL opcode and interleave fast locals offsets. #26396 bpo-43693: Revert commits 2c1e2583fdc4db6b43d163239ea42b0e8394171f and b2bf2bc1ece673d387341e06c8d3c2bc6e259747  #26530 bpo-43693: Un-revert commits 2c1e258 and b2bf2bc. #26577 bpo-43693: Eliminate unused "fast locals". #26587 bpo-43693: Silence some compiler warnings. #26588 Revert "bpo-43693: Add the MAKE_CELL opcode and interleave fast locals offsets. (gh-26396)" #26597 bpo-43693: Un-revert commit f3fa63e. #26609 bpo-43693: Do not check co_cell2arg if a non-cell offset. #26626 bpo-43693: Turn localspluskinds into an object #26749 bpo-43693 Get rid of CO_NOFREE -- it's unused #26839

^{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 = None
closed_at = None
created_at = <Date 2021-04-01.11:33:50.457>
labels = ['interpreter-core', 'performance']
title = 'Logically merge cell and locals array. They are already contiguous in memory'
updated_at = <Date 2021-07-21.18:07:49.033>
user = 'https://github.com/markshannon'

bugs.python.org fields:

activity = <Date 2021-07-21.18:07:49.033>
actor = 'gvanrossum'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Interpreter Core']
creation = <Date 2021-04-01.11:33:50.457>
creator = 'Mark.Shannon'
dependencies = []
files = []
hgrepos = []
issue_num = 43693
keywords = ['patch']
message_count = 32.0
messages = ['389974', '390069', '390247', '390253', '394455', '394556', '395014', '395059', '395097', '395098', '395102', '395105', '395137', '395273', '395294', '395297', '395317', '395318', '395319', '395321', '395326', '395329', '395331', '395332', '395338', '395365', '395448', '395899', '396015', '396288', '396433', '397956']
nosy_count = 8.0
nosy_names = ['gvanrossum', 'rhettinger', 'ncoghlan', 'Mark.Shannon', 'eric.snow', 'serhiy.storchaka', 'Guido.van.Rossum', 'pablogsal']
pr_nums = ['25152', '26216', '26258', '26364', '26375', '26388', '26396', '26530', '26577', '26587', '26588', '26597', '26609', '26626', '26749', '26839']
priority = 'normal'
resolution = None
stage = 'needs patch'
status = 'open'
superseder = None
type = 'performance'
url = 'https://bugs.python.org/issue43693'
versions = []

Linked PRs

• gh-87859: Track Code Object Local Kinds For Arguments #132980

Activity

1. markshannon commented on Apr 1, 2021

   

   markshannon

   on Apr 1, 2021

   MemberAuthor

   More actions

   In the interpreter and compiler, the "fast" locals array and cells array are treated separately. By merging them in the compiler, the interpreter can be simplified a bit.
2. markshannon

   self-assigned this

   on Apr 1, 2021
3. markshannon

   added

   interpreter-core(Objects, Python, Grammar, and Parser dirs)(Objects, Python, Grammar, and Parser dirs)

   performancePerformance or resource usagePerformance or resource usage

   on Apr 1, 2021
4. markshannon

   self-assigned this

   on Apr 1, 2021
5. markshannon

   added

   interpreter-core(Objects, Python, Grammar, and Parser dirs)(Objects, Python, Grammar, and Parser dirs)

   performancePerformance or resource usagePerformance or resource usage

   on Apr 1, 2021
6. rhettinger commented on Apr 2, 2021

   

   rhettinger

   on Apr 2, 2021

   Contributor

   More actions

   This doesn't look simpler to me. We will forever lose mental clock cycles disentangling the locals and cells. So, I don't think they should be commingled.
7. gvanrossum commented on Apr 5, 2021

   

   gvanrossum

   on Apr 5, 2021

   Member

   More actions

   To me it looks simpler though. The locals and cells are already stored in a single array, f_localsplus (which also contains the evaluation stack). There are various complications in ceval.c to translate cell indexes to indexes in this array (ein particular the extra local variable 'freevars', which weighs down the stack frame).

   Making the interpreter simpler by moving things to the compiler also makes it easier for the C compiler to optimize the code of the interpreter better.
8. serhiy-storchaka commented on Apr 5, 2021

   

   serhiy-storchaka

   on Apr 5, 2021

   Member

   More actions

   LGTM.
9. markshannon commented on May 26, 2021

   

   markshannon

   on May 26, 2021

   MemberAuthor

   More actions

   New changeset 6cc800d by Eric Snow in branch 'main':
   bpo-43693: Clean up the PyCodeObject fields. (GH-26364)
   6cc800d
10. ericsnowcurrently commented on May 27, 2021

    

    ericsnowcurrently

    on May 27, 2021

    Member

    More actions

    New changeset 9f494d4 by Eric Snow in branch 'main':
    bpo-43693: Add _PyCode_New(). (gh-26375)
    9f494d4
11. ericsnowcurrently commented on Jun 3, 2021

    

    ericsnowcurrently

    on Jun 3, 2021

    Member

    More actions

    New changeset 2c1e258 by Eric Snow in branch 'main':
    bpo-43693: Add new internal code objects fields: co_fastlocalnames and co_fastlocalkinds. (gh-26388)
    2c1e258
12. ericsnowcurrently commented on Jun 4, 2021

    

    ericsnowcurrently

    on Jun 4, 2021

    Member

    More actions

    New changeset b2bf2bc by Mark Shannon in branch 'main':
    bpo-43693: Compute deref offsets in compiler (gh-25152)
    b2bf2bc
13. pablogsal commented on Jun 4, 2021

    

    pablogsal

    on Jun 4, 2021

    Member

    More actions

    Unfortunately, commit 2c1e258 has broken all the refleak buildbots. Example failure:

    BISECTION:

    c1e2583fdc4db6b43d163239ea42b0e8394171f is the first bad commit
    commit 2c1e258
    Author: Eric Snow <[email protected]>
    Date: Thu Jun 3 10:28:27 2021 -0600

    bpo-43693: Add new internal code objects fields: co_fastlocalnames and co_fastlocalkinds. (gh-26388)
    

    https://buildbot.python.org/all/#/builders/384/builds/50/steps/5/logs/stdio

    As this is affecting all test, I am proceeding with a revert of commit 2c1e258 directly to not mask other issues.
14. 18 remaining items

15. ericsnowcurrently commented on Jun 15, 2021

    

    ericsnowcurrently

    on Jun 15, 2021

    Member

    More actions

    New changeset ac38a9f by Eric Snow in branch 'main':
    bpo-43693: Eliminate unused "fast locals". (gh-26587)
    ac38a9f
16. markshannon

    assigned

    ericsnowcurrently

    and unassigned

    markshannon

    on Jun 17, 2021
17. ericsnowcurrently commented on Jun 17, 2021

    

    ericsnowcurrently

    on Jun 17, 2021

    Member

    More actions

    FWIW, I've wrapped up the key parts that I wanted to get done here
    (co_localplusnames/kinds, MAKE_CELL, eliminate unused fast local
    for arg cells). I'm leaving this open for now as there are a few
    things I didn't do that seem part of the original intention of this
    issue:

    • fully interleave the cells with the locals in their "natural" order
      (rather than only interleaving arg cells)
    • update the compiler to track names/kinds rather than computing
      them after the fact during the assembler step
      (this will allow us to remove a decent amount of code)
    • track the specific arg kinds in localspluskinds
      (this should allow us to make _PyEval_MakeFrameVector() simpler
      and a bit more efficient)
18. gvanrossum commented on Jun 21, 2021

    

    gvanrossum

    on Jun 21, 2021

    Member

    More actions

    New changeset 355f5dd by Guido van Rossum in branch 'main':
    bpo-43693: Turn localspluskinds into an object (GH-26749)
    355f5dd
19. gvanrossum commented on Jun 23, 2021

    

    gvanrossum

    on Jun 23, 2021

    Member

    More actions

    New changeset 769d7d0 by Guido van Rossum in branch 'main':
    bpo-43693 Get rid of CO_NOFREE -- it's unused (GH-26839)
    769d7d0
20. ericsnowcurrently

    unassigned

    ericsnowcurrently

    on Jul 21, 2021
21. gvanrossum commented on Jul 21, 2021

    

    gvanrossum

    on Jul 21, 2021

    Member

    More actions

    Another task seems to have appeared, Nick Coghlan would like the typedef to be renamed to PyLocal_VarKind, and the getter/setter similarly.
22. ezio-melotti

    transferred this issue fromon Apr 10, 2022
23. markshannon commented on May 2, 2022

    

    markshannon

    on May 2, 2022

    MemberAuthor

    More actions

    I don't think there is anything left to do here, so closing.
24. markshannon

    closed this as completedon May 2, 2022
25. ericsnowcurrently

    added a commit that references this issue on Apr 29, 2025

    gh-87859: Track Code Object Local Kinds For Arguments (gh-132980)

    219d8d2

Issue actions