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

bpo-40222: "Zero cost" exception handling #25729

Merged
merged 45 commits into from May 7, 2021

Conversation

@markshannon
Copy link
Contributor

@markshannon markshannon commented Apr 29, 2021

For example, this function:

def f():
    try:
        g(0)
    except:
        return "fail"

compiles as follows on main:

  2           0 SETUP_FINALLY            7 (to 16)

  3           2 LOAD_GLOBAL              0 (g)
              4 LOAD_CONST               1 (0)
              6 CALL_FUNCTION            1
              8 POP_TOP
             10 POP_BLOCK
             12 LOAD_CONST               0 (None)
             14 RETURN_VALUE

  4     >>   16 POP_TOP
             18 POP_TOP
             20 POP_TOP

  5          22 POP_EXCEPT
             24 LOAD_CONST               3 ('fail')
             26 RETURN_VALUE

with this PR it compiles as follows:

  2           0 NOP

  3           2 LOAD_GLOBAL              0 (g)
              4 LOAD_CONST               1 (0)
              6 CALL_FUNCTION            1
              8 POP_TOP
             10 LOAD_CONST               0 (None)
             12 RETURN_VALUE
        >>   14 PUSH_EXC_INFO

  4          16 POP_TOP
             18 POP_TOP
             20 POP_TOP

  5          22 POP_EXCEPT
             24 LOAD_CONST               2 ('fail')
             26 RETURN_VALUE
        >>   28 POP_EXCEPT_AND_RERAISE
ExceptionTable:
  2 to 8 -> 14 [0]
  14 to 20 -> 28 [3] lasti

This is not quite zero-cost at the moment, as it leaves a NOPs for each try, and possibly a few other.

Removing NOPs that are on a line by themselves will require changes to the line number table, which has nothing to do with exception handling and I don't want to mix the two PRs.

Although the code is slightly longer overall, there is less work done on the non-exceptional path, one NOP versus a SETUP_FINALLY and POP_BLOCK.

Not only is there a reduction in work done in the bytecode, but in the size of frames is reduced a lot:

def frame_size():
    return sys.getsizeof(sys._getframe())

On master:

>>> frame_size()
400

with this PR:

>>> frame_size()
152

https://bugs.python.org/issue40222

@markshannon markshannon force-pushed the faster-cpython:zero-cost-except branch from 2e36c09 to 062ffa4 Apr 30, 2021
@markshannon markshannon changed the title "Zero cost" exception handling bpo-40222: "Zero cost" exception handling Apr 30, 2021
@ericsnowcurrently ericsnowcurrently self-requested a review May 6, 2021
Copy link
Member

@ericsnowcurrently ericsnowcurrently left a comment

Mostly LGTM, but with a bunch of small comments to sort out first

My only big request would be a bit more explanation somewhere of how the zero-cost exception handling works. I didn't have much context when reading through frameobject.c, as you may note by a number of "why did this change" comments. 🙂

Also, have you been able to benchmark this? My guess is that it will be faster and leaner generally, but a little slower when handling exceptions.

Include/cpython/frameobject.h Show resolved Hide resolved
Lib/importlib/_bootstrap_external.py Show resolved Hide resolved
PyAPI_FUNC(void) PyFrame_BlockSetup(PyFrameObject *, int, int, int);
PyAPI_FUNC(PyTryBlock *) PyFrame_BlockPop(PyFrameObject *);
Comment on lines -82 to -83

This comment has been minimized.

@ericsnowcurrently

ericsnowcurrently May 6, 2021
Member

Given that these were part of the public C-API, it would probably be worth adding a note to the What's New doc about their removal. I suppose the same may be true of f_blockstack, though it's less likely that matters.

Python/compile.c Show resolved Hide resolved
Python/compile.c Outdated Show resolved Hide resolved
if (!handler->v.ExceptHandler.type && i < n-1) {
return compiler_error(c, "default 'except:' must be last");
}
Comment on lines +3217 to +3219

This comment has been minimized.

@ericsnowcurrently

ericsnowcurrently May 6, 2021
Member

Is this another case where you happened to notice it was incorrect, or is this change due to this PR?

This comment has been minimized.

@markshannon

markshannon May 6, 2021
Author Contributor

I think this was incorrect before. I'll see if it makes sense to apply it to 3.10b.

Python/compile.c Outdated Show resolved Hide resolved
Python/compile.c Show resolved Hide resolved
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented May 6, 2021

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

And if you don't make the requested changes, you will be put in the comfy chair!

@markshannon markshannon removed the request for review from python/windows-team May 6, 2021
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented May 6, 2021

🤖 New build scheduled with the buildbot fleet by @markshannon for commit e1d6e1e 🤖

If you want to schedule another build, you need to add the "🔨 test-with-buildbots" label again.

@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented May 7, 2021

🤖 New build scheduled with the buildbot fleet by @markshannon for commit b92ada2 🤖

If you want to schedule another build, you need to add the "🔨 test-with-buildbots" label again.

@markshannon
Copy link
Contributor Author

@markshannon markshannon commented May 7, 2021

The failure on buildbot/AMD64 Arch Linux Asan PR is a pre-existing failure.
The failure on buildbot/PPC64LE Fedora Stable seems to be some sort of race condition in concurrent_futures

@markshannon markshannon merged commit adcd220 into python:main May 7, 2021
12 checks passed
12 checks passed
@github-actions
Docs
Details
@github-actions
Check for source changes
Details
@github-actions
Check if generated files are up to date
Details
@github-actions
Windows (x86)
Details
@github-actions
Windows (x64)
Details
@github-actions
macOS
Details
@github-actions
Ubuntu
Details
@github-actions
Ubuntu SSL tests with OpenSSL
Details
Azure Pipelines PR #20210507.22 succeeded
Details
@travis-ci
Travis CI - Pull Request Build Passed
Details
@bedevere-bot
bedevere/issue-number Issue number 40222 found
Details
@bedevere-bot
bedevere/news News entry found in Misc/NEWS.d
@markshannon markshannon deleted the faster-cpython:zero-cost-except branch May 7, 2021
@pablogsal
Copy link
Member

@pablogsal pablogsal commented May 10, 2021

The failure on buildbot/AMD64 Arch Linux Asan PR is a pre-existing failure.
The failure on buildbot/PPC64LE Fedora Stable seems to be some sort of race condition in concurrent_futures

@markshannon Unfortunately the ASAN failure is legit. Check the BPO issue for more details.

PyAPI_FUNC(PyCodeObject *) PyCode_New(
int, int, int, int, int, PyObject *, PyObject *,
PyObject *, PyObject *, PyObject *, PyObject *,
PyObject *, PyObject *, int, PyObject *);
PyObject *, PyObject *, int, PyObject *, PyObject *);

PyAPI_FUNC(PyCodeObject *) PyCode_NewWithPosOnlyArgs(
int, int, int, int, int, int, PyObject *, PyObject *,
PyObject *, PyObject *, PyObject *, PyObject *,
PyObject *, PyObject *, int, PyObject *);
PyObject *, PyObject *, int, PyObject *, PyObject *);
Comment on lines 118 to +126

This comment has been minimized.

@scoder

scoder May 11, 2021
Contributor

This C-API change seems worth mentioning somewhere.

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants