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

Wordcode, part 2 #71316

Closed
serhiy-storchaka opened this issue May 26, 2016 · 36 comments
Closed

Wordcode, part 2 #71316

serhiy-storchaka opened this issue May 26, 2016 · 36 comments
Labels
3.10 interpreter-core Interpreter core (Objects, Python, Grammar, and Parser dirs) release-blocker type-feature A feature request or enhancement

Comments

@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented May 26, 2016

BPO 27129
Nosy @rhettinger, @db3l, @vstinner, @markshannon, @berkerpeksag, @serhiy-storchaka, @efahl, @serprex, @pablogsal, @godaygo, @sweeneyde
PRs
  • bpo-27129: Use instruction offsets, not byte offsets, in bytecode and internally. #25069
  • bpo-27129: Update magic numbers and bootstrapping for GH-25069 #25172
  • Files
  • wordcode2.patch
  • word-jump-offsets.patch
  • wordcode-refactor.patch
  • wordcode-refactor2.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 = None
    closed_at = <Date 2021-04-05.16:25:09.267>
    created_at = <Date 2016-05-26.13:13:07.796>
    labels = ['interpreter-core', 'type-feature', '3.10', 'release-blocker']
    title = 'Wordcode, part 2'
    updated_at = <Date 2021-04-05.16:25:09.265>
    user = 'https://github.com/serhiy-storchaka'

    bugs.python.org fields:

    activity = <Date 2021-04-05.16:25:09.265>
    actor = 'pablogsal'
    assignee = 'none'
    closed = True
    closed_date = <Date 2021-04-05.16:25:09.267>
    closer = 'pablogsal'
    components = ['Interpreter Core']
    creation = <Date 2016-05-26.13:13:07.796>
    creator = 'serhiy.storchaka'
    dependencies = []
    files = ['43015', '43040', '43308', '43333']
    hgrepos = []
    issue_num = 27129
    keywords = ['patch']
    message_count = 36.0
    messages = ['266431', '266435', '266446', '266447', '266489', '266490', '266556', '266604', '266611', '267880', '268132', '268142', '268143', '268145', '275771', '275773', '275780', '275781', '276044', '276049', '276055', '315266', '389707', '389991', '390017', '390024', '390025', '390082', '390085', '390088', '390099', '390151', '390152', '390157', '390178', '390242']
    nosy_count = 12.0
    nosy_names = ['rhettinger', 'db3l', 'vstinner', 'Mark.Shannon', 'python-dev', 'berker.peksag', 'serhiy.storchaka', 'eric.fahlgren', 'Demur Rumed', 'pablogsal', 'godaygo', 'Dennis Sweeney']
    pr_nums = ['25069', '25172']
    priority = 'release blocker'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue27129'
    versions = ['Python 3.10']

    @serhiy-storchaka
    Copy link
    Member Author

    serhiy-storchaka commented May 26, 2016

    This is the second stage of converting to wordcode (bpo-26647).

    Proposed patch makes bytecodecode consisting of code units (16-bit words) instead of bytes. It includes following changes:

    • Changes meaning of jump offsets. They counts not bytes, but code units. This extends the range addressed by short commands (from 256 bytes to 256 words) and simplifies ceval.c.

    • Changes f_lasti, tb_lasti etc to count code units instead of bytes.

    • Changes disassembler to show addresses in code units, not bytes.

    • Refactores the code.

    These changes break compatibility (already broken by switching to 16-bit bytecode). The first one breaks compatibility with compiled bytecode and needs incrementing the magic number.

    @serhiy-storchaka serhiy-storchaka added interpreter-core Interpreter core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement labels May 26, 2016
    @vstinner
    Copy link
    Member

    vstinner commented May 26, 2016

    Changes f_lasti, tb_lasti etc to count code units instead of bytes.

    I asked Demur to not break f_lasti. I don't understand if this change
    breaks applications using f_lasti or not.

    For example, asyncio/coroutines.py uses:

                if caller.f_code.co_code[caller.f_lasti] != _YIELD_FROM:
                    value = value[0]

    Does this code still work with your change?

    Maybe this code is already broken by wordcode, but it doesn't really
    matter since it should only be used on the exact version 3.4.0. The
    code works around a bug in Python 3.4.0, fixed in Python 3.4.1 (issue
    bpo-21209).

    Other known users of f_lasti are development tools like debuggers
    (pdb), profilers, code coverage, etc. We should check these tools.

    @brettcannon
    Copy link
    Member

    brettcannon commented May 26, 2016

    So is avoiding changing f_lasti just to minimize breakage of tools? Aren't they going to have to update to support the wordcode changes anyway?

    @serhiy-storchaka
    Copy link
    Member Author

    serhiy-storchaka commented May 26, 2016

    The patch contains the change of Lib/asyncio/coroutines.py. This is the only change in Python code besides the dis module.

    I can keep f_lasti to be twice the number of instructions, but this will complicate the patch. The simplest way perhaps is to convert this read-only attribute to the property that multiplies internal f_lasti by 2.

    @serprex
    Copy link
    Mannequin

    serprex mannequin commented May 27, 2016

    https://github.com/search?q=f_lasti&type=Code

    Popular use of f_lasti is checking it for -1, checking the instruction at the byte offset of f_lasti, checking the argument with code[f_lasti+1] (Some bad code checking f_lasti+3 which'll break with 3.6)

    abarnert discussed how bytecode should be typed to Python code. Ideally it'd be typed as a "(instruction, arg)" tuple. He considered creating a "words" type similar to "bytes" but with 16 bit values. It's a bit niche to introduce a builtin for. So if the co_code object is remaining a bytes object then it seems intuitive to keep f_lasti as a bytes offset. Clashes with jump offsets no longer being a bytes offset even in Python code tho

    In reality most of the results on github all seem to be copying a few distinct uses. So maybe backwards compatibiltiy isn't so important

    Other search https://searchcode.com/?q=f_lasti&loc=0&loc2=10000&src=3&src=7&src=1&lan=19 doesn't produce many results either

    @vstinner
    Copy link
    Member

    vstinner commented May 27, 2016

    if the co_code object is remaining a bytes object then it seems intuitive to keep f_lasti as a bytes offset

    Right.

    In reality most of the results on github all seem to be copying a few distinct uses. So maybe backwards compatibiltiy isn't so important

    Backwards compatibiltiy is important.

    @serhiy-storchaka
    Copy link
    Member Author

    serhiy-storchaka commented May 28, 2016

    Here is a patch that implements only the first change -- makes jump offsets be in 16-bit units, not bytes. This is minimal change, it doesn't include refactoring.

    @rhettinger
    Copy link
    Contributor

    rhettinger commented May 29, 2016

    I don't see how this is a simplification. The additional /2 and *2 on the affected lines makes the code a little harder to reason about and it loses some of the cleanness achieved by the last patch. To me, it also increases conceptual complexity because INSTR_OFFSET() no longer gives the byte address adjustment.

    @serhiy-storchaka
    Copy link
    Member Author

    serhiy-storchaka commented May 29, 2016

    word-jump-offsets.patch doesn't simplify the code, but rather complicates it, because this is minimal patch that doesn't include the refactoring. The main benefit of this patch is that it extends the range addressed by short jump instruction. The other benefit is that the target of jump instruction is now always point at instruction boundary.

    wordcode2.patch includes refactoring and other changes:

    • Changes jump offsets. This extends addressed range and can make co_code more compact.
    • Changes co_lnotab. This can make co_lnotab more compact.
    • Changes f_lasti.
    • Changes the disassembler.
    • Refactors the C code (makes it codeunit-oriented instead of byte-oriented). Simplifies the code complicated by other changes.

    I can provide these steps by separate patches (word-jump-offsets.patch is the first of them), but every separate patch can temporary complicate the code. Three of these changes (except the disassembler changing and refactoring) break the compatibility of pyc-files and need incrementing the magic number.

    @serhiy-storchaka
    Copy link
    Member Author

    serhiy-storchaka commented Jun 8, 2016

    Here is a patch that just refactors the code. It renames OPCODE and OPCODE to _Py_OPCODE and _Py_OPCODE and moves them to code.h for reusing in other files. Introduces _Py_CODEUNIT as an alias to unsigned short (if it is 16-bit, otherwise a compile error is raised). Makes compiler and peepholer to operate with _Py_CODEUNIT units instead of bytes. Replaces or scale magic numbers with sizeof(_Py_CODEUNIT). Adds fill_nops() for filling specified region with NOPs. Decreases memory consumption for peepholer (doesn't allocate memory for unused odd addresses).

    @rhettinger
    Copy link
    Contributor

    rhettinger commented Jun 10, 2016

    I really don't think any of this should be done at all. The current code is clean and fast (and to my eyes at least is very readable).

    @serhiy-storchaka
    Copy link
    Member Author

    serhiy-storchaka commented Jun 10, 2016

    This is not just about cleaning (to my eyes current code is not very readable, and I read it many times, perhaps more times than any other core developer in last months). There are other benefits. Changing jump offsets allows to get rid of EXTENDED_ARGs for the part of jump opcodes. Changing lnotab makes it more compact and allows the peepholer to optimize the code that it rejects now. Refactoring includes the change that decreases memory consumption of the peepholer (from 4 bytes per bytecode byte to 2 bytes per bytecode byte). Changing jump offsets together with changing f_lasti removes redundant multiplications and divisions by 2. Separate changes can complicate some parts of code, but next changes removes this complication. Only all changes together achieve maximal cleanness.

    I think that converting to wordcode is not complete without these changes. I approved the wordcode patch only having in mind following changes. It is more painless to make all changes in one Python release than break compatibility during few releases.

    @serhiy-storchaka
    Copy link
    Member Author

    serhiy-storchaka commented Jun 10, 2016

    update patch replaces yet few magic constants.

    @serprex
    Copy link
    Mannequin

    serprex mannequin commented Jun 10, 2016

    The patches LGTM & seem to be implementation of follow up ideas outlined in the first portion. It'd be good to verify that benchmarks are relatively unaffected

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 11, 2016

    New changeset dd046963bd42 by Serhiy Storchaka in branch 'default':
    Issue bpo-27129: Replaced wordcode related magic constants with macros.
    https://hg.python.org/cpython/rev/dd046963bd42

    @berkerpeksag
    Copy link
    Member

    berkerpeksag commented Sep 11, 2016

    From http://buildbot.python.org/all/builders/s390x%20Debian%203.x/builds/1811/steps/compile/logs/stdio (I also saw the same compile error on another Linux boxes)

    _freeze_importlib: Python/peephole.c:524: PyCode_Optimize: Assertion `((codestr[i]) >> 8) == 100' failed.
    Makefile:735: recipe for target 'Python/importlib.h' failed
    make: *** [Python/importlib.h] Aborted

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 11, 2016

    New changeset b49a938eaa31 by Serhiy Storchaka in branch 'default':
    Fixed refactoring bug in dd046963bd42 (bpo-27129).
    https://hg.python.org/cpython/rev/b49a938eaa31

    @serhiy-storchaka
    Copy link
    Member Author

    serhiy-storchaka commented Sep 11, 2016

    Thanks Berker!

    @vstinner
    Copy link
    Member

    vstinner commented Sep 12, 2016

    This issue can now be closed, no?

    @serhiy-storchaka
    Copy link
    Member Author

    serhiy-storchaka commented Sep 12, 2016

    No, only the simpler and safer part was committed. I divided the original patch on four parts, but since the code was significantly evolved, they no longer applied clearly.

    @vstinner
    Copy link
    Member

    vstinner commented Sep 12, 2016

    Serhiy Storchaka added the comment:

    No, only the simpler and safer part was committed. I divided the original patch on four parts, but since the code was significantly evolved, they no longer applied clearly.

    Oh ok. I saw that a change was pushed, I didn't read the history of the issue.

    I should take a look after the beta1 release.

    @godaygo
    Copy link
    Mannequin

    godaygo mannequin commented Apr 13, 2018

    Hello, what is the future of this patch? Such a feeling that the transition to wordcode is still in some half-way state.

    @markshannon
    Copy link
    Member

    markshannon commented Mar 29, 2021

    frame.f_lasti and traceback.tb_lasti are best left as byte offsets.
    There is no guarantee that we won't go back to variable length instructions.
    For example, a "LONG_JUMP" instruction which is 4 bytes long and takes a 3 byte offset might well be a worthwhile extension.

    However, changing bytecode offsets and the internal representation of frame.f_lasti will reduce the number of "EXTENDED_ARG"s by 60% or more and makes interpreter dispatch a tad more efficient.

    @markshannon
    Copy link
    Member

    markshannon commented Apr 1, 2021

    New changeset fcb55c0 by Mark Shannon in branch 'master':
    bpo-27129: Use instruction offsets, not byte offsets, in bytecode and internally. (GH-25069)
    fcb55c0

    @db3l
    Copy link
    Contributor

    db3l commented Apr 1, 2021

    Note that this commit appears to be causing exceptions for the Win10 buildbot, failing the PyCode_Addr2Line assertion in codeobject.c line 1252.

    The assertion seems to pop up at differing points during each test run, but the builder has yet to complete a full test run successfully.

    @markshannon
    Copy link
    Member

    markshannon commented Apr 2, 2021

    That assertion is correct, and hasn't changed.

    Do you have a traceback?
    The buildbot just shows the assertion message with no context.

    @db3l
    Copy link
    Contributor

    db3l commented Apr 2, 2021

    Unfortunately, not at the moment - what's in the buildbot log is what's available. The RTL assertion aborts the process.

    The tests involved (such as test_clinic) do seem reproducible in a few separate tries, though again, all they do is terminate.

    As the assertion should be correct, I'm guessing it's reflecting an earlier corruption. There's some other oddities, such as the "Leaf" related failures in test_peg_generator that showed up at the same time, in case that offers any hint. Since Leaf and StringLeaf are almost next to each other in grammar.py I can't see how it can be undefined.

    The worker only has the core build tools version of VS so can't directly debug this further locally. I can look into using a different machine to try to get some details, but I'm not sure as to timing.

    @vstinner
    Copy link
    Member

    vstinner commented Apr 2, 2021

    I get two crashes on Windows with Python built in debug mode:

    • I got a crash. I wasn't sure if it was an issue of incremental build, so I rebuilt Python.
    • On a fresh build, Python crashed on the CALL_FUNCTION_KW opcode, when loading names, names was equal to 0xFFFFFFFFFFFFFFFF:
                names = POP();
                assert(PyTuple_Check(names)); <=== HERE

    Moreover, f->f_code was equal to 0xCBCBCBCBCBCBCBCB. But it was really weird. I added assertion to ensure that f->f_code was not equal 0xCBCBCBCBCBCBCBCB: the assertion didn't fail.

    • I ran "git clean -fdx" and built again Python. This time, it went fine, moreover the whole test suite passed cleanly!? "== Tests result: SUCCESS ==" and "386 tests OK."

    I build Python with:

    PCbuild\build.bat -d -p x64 -e

    @db3l
    Copy link
    Contributor

    db3l commented Apr 2, 2021

    Ah, Victor, that helps. I was having trouble reproducing the problem on a different system. I was suspecting a small difference in compiler version, but I hadn't considered it being because I started fresh.

    From what I can see, a particular build tree can be successful if it remains on either side of the instruction commit, but you can't cross the boundary - in either direction. There's clearly some build artifact not being reset properly that gets out of sync. I've been able to create odd name errors even in older commits as long as the first one I build is at or after the instruction commit.

    But a pristine checkout on the buildbot of the latest master works, as does a git clean on the tree.

    I always use the buildbot scripts for building and they invoke a full clean first. So either the clean process on Windows is missing something, or there's an artifact that is supposed to be kept in sync in the source tree itself that isn't. I'm not familiar enough with the internals to guess at which yet. I suppose given that it's not a problem on other buildbots argues for the clean issue, although I suppose it could also be something that is only kept in the tree to benefit a subset of systems, like Windows.

    (While resetting the checkouts on the buildbot should therefore fix the current exceptions, I'm going to leave that alone for the moment, since that leaves it positioned to confirm any subsequent fix)

    @db3l
    Copy link
    Contributor

    db3l commented Apr 2, 2021

    I'm out of time for a bit, but it appears that the root issue is old pyc files in Tools/clinic/__pycache__ that aren't removed during a clean process, and appear to be the source of all of the errors. Manually pruning that folder fixes things.

    I believe the regular (non-Windows) makefile automatically prunes all __pycache__ folders in the tree during clean which is probably why that's not an issue on other systems.

    @db3l
    Copy link
    Contributor

    db3l commented Apr 2, 2021

    I've opened issue bpo-43709 for fixing the buildbot clean script under Windows. It needs to clean the Tools and Parser trees, not just Lib (and there are a few other folders involved besides clinic)

    @vstinner
    Copy link
    Member

    vstinner commented Apr 3, 2021

    New changeset fcb55c0 by Mark Shannon in branch 'master':
    bpo-27129: Use instruction offsets, not byte offsets, in bytecode and internally. (GH-25069)

    This change broke buildbots, please revert it to repair buildbots.

    More and more people are affected: https://bugs.python.org/issue43719

    @db3l
    Copy link
    Contributor

    db3l commented Apr 3, 2021

    I don't think reverting the commit at this point would necessarily be helpful. While it might fix some systems, it could newly break anyone who happened to do their first build since the commit was in place.

    I didn't want to bug anyone over the weekend, but I've got a PR as part of issue bpo-43709 that I believe would fix this going forward, if anyone with access might have an opportunity to review it.

    @sweeneyde
    Copy link
    Member

    sweeneyde commented Apr 3, 2021

    I notice this in _bootstrap_external.py: the magic number did not get changed, only the comment:

    # Python 3.10a2 3433 (RERAISE restores f_lasti if oparg != 0)
    # Python 3.10a6 3434 (PEP-634: Structural Pattern Matching)
    # Python 3.10a7 3435 Use instruction offsets (as opposed to byte offsets).

    #
    # MAGIC must change whenever the bytecode emitted by the compiler may no
    # longer be understood by older implementations of the eval loop (usually
    # due to the addition of new opcodes).
    #
    # Whenever MAGIC_NUMBER is changed, the ranges in the magic_values array
    # in PC/launcher.c must also be updated.
    MAGIC_NUMBER = (3434).to_bytes(2, 'little') + b'\r\n'
    _RAW_MAGIC_NUMBER = int.from_bytes(MAGIC_NUMBER, 'little')  # For import.c

    @markshannon
    Copy link
    Member

    markshannon commented Apr 4, 2021

    New changeset c368ce7 by Dennis Sweeney in branch 'master':
    bpo-27129: Update magic numbers and bootstrapping for #69255 (GH-25172)
    c368ce7

    @pablogsal
    Copy link
    Member

    pablogsal commented Apr 5, 2021

    Closing as this is fixed. Feel free to reopen if there is something missing

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    miss-islington pushed a commit to miss-islington/cpython that referenced this issue Oct 7, 2022
    …rguments (pythonGH-95798)
    
    (cherry picked from commit 6592a62)
    
    Co-authored-by: Christopher Chianelli <cchianel@redhat.com>
    miss-islington added a commit that referenced this issue Oct 7, 2022
    …ts (GH-95798)
    
    (cherry picked from commit 6592a62)
    
    Co-authored-by: Christopher Chianelli <cchianel@redhat.com>
    Christopher-Chianelli added a commit to Christopher-Chianelli/cpython that referenced this issue Oct 7, 2022
    … jump arguments (pythonGH-95798).
    
    (cherry picked from commit 6592a62)
    
    Co-authored-by: Christopher Chianelli <cchianel@redhat.com>
    Christopher-Chianelli added a commit to Christopher-Chianelli/cpython that referenced this issue Oct 7, 2022
    … jump arguments (pythonGH-95798).
    
    (cherry picked from commit 6592a62)
    
    Co-authored-by: Christopher Chianelli <cchianel@redhat.com>
    iritkatriel pushed a commit to iritkatriel/cpython that referenced this issue Oct 7, 2022
    iritkatriel pushed a commit to iritkatriel/cpython that referenced this issue Oct 7, 2022
    iritkatriel pushed a commit that referenced this issue Oct 7, 2022
    …arguments (GH-95798). (GH-98029)
    
    (cherry picked from commit 6592a62)
    
    Co-authored-by: Christopher Chianelli <cchianel@redhat.com>
    mpage pushed a commit to mpage/cpython that referenced this issue Oct 11, 2022
    pablogsal pushed a commit that referenced this issue Oct 22, 2022
    …ts (GH-95798)
    
    (cherry picked from commit 6592a62)
    
    Co-authored-by: Christopher Chianelli <cchianel@redhat.com>
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.10 interpreter-core Interpreter core (Objects, Python, Grammar, and Parser dirs) release-blocker type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    10 participants