bpo-40222: "Zero cost" exception handling #25729
Conversation
…o restore when re-raising.
2e36c09
to
062ffa4
|
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. |
Misc/NEWS.d/next/Core and Builtins/2021-04-30-15-48-36.bpo-40222.j3VxeX.rst
Outdated
Show resolved
Hide resolved
Misc/NEWS.d/next/Core and Builtins/2021-04-30-15-48-36.bpo-40222.j3VxeX.rst
Outdated
Show resolved
Hide resolved
| PyAPI_FUNC(void) PyFrame_BlockSetup(PyFrameObject *, int, int, int); | ||
| PyAPI_FUNC(PyTryBlock *) PyFrame_BlockPop(PyFrameObject *); |
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.
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.
| if (!handler->v.ExceptHandler.type && i < n-1) { | ||
| return compiler_error(c, "default 'except:' must be last"); | ||
| } |
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?
Is this another case where you happened to notice it was incorrect, or is this change due to this PR?
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.
I think this was incorrect before. I'll see if it makes sense to apply it to 3.10b.
|
When you're done making the requested changes, leave the comment: And if you don't make the requested changes, you will be put in the comfy chair! |
…e evaluation stack, not the block stack.
|
If you want to schedule another build, you need to add the " |
|
If you want to schedule another build, you need to add the " |
|
The failure on |
@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 *); |
scoder
May 11, 2021
Contributor
This C-API change seems worth mentioning somewhere.
This C-API change seems worth mentioning somewhere.
For example, this function:
compiles as follows on main:
with this PR it compiles as follows:
This is not quite zero-cost at the moment, as it leaves a
NOPs for eachtry, 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
NOPversus aSETUP_FINALLYandPOP_BLOCK.Not only is there a reduction in work done in the bytecode, but in the size of frames is reduced a lot:
On master:
with this PR:
https://bugs.python.org/issue40222