Exceptions.c PyCode_New -> PyCode_NewEmpty #4479
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
With reference to cython#4365 (comment) in Python 3 PyCode_NewEmpty does everything we need to set exception traceback code objects. This change is probably only a real improvement on Py3.11 onwards (where the replacement for PyCode_new is currently pretty slow). On earlier version it'll probably be fairly similar (maybe one extra allocation for the cline case?)
scoder
reviewed
Nov 24, 2021
Cython/Utility/Exceptions.c
Outdated
| #if PY_MAJOR_VERSION < 3 | ||
| PyObject *py_srcfile = 0; |
There was a problem hiding this comment.
I know, it's not always been done before, but I'd rather see the NULL-pointer spelled as NULL.
Suggested change
| PyObject *py_srcfile = 0; | |
| PyObject *py_srcfile = NULL; |
Cython/Utility/Exceptions.c
Outdated
| #else | ||
| py_code = PyCode_NewEmpty(filename, funcname, py_line); | ||
| #endif | ||
| Py_XDECREF(py_funcname); // XDECREF is it's only set on Py3 if cline |
There was a problem hiding this comment.
Suggested change
| Py_XDECREF(py_funcname); // XDECREF is it's only set on Py3 if cline | |
| Py_XDECREF(py_funcname); // XDECREF since it's only set on Py3 if cline |
Cython/Utility/Exceptions.c
Outdated
| #endif | ||
| } | ||
| #if PY_MAJOR_VERSION < 3 | ||
| if (!py_funcname) goto bad; |
There was a problem hiding this comment.
This can be moved into the else case now.
There was a problem hiding this comment.
It was covering two cases (which is why I left it there). With that said, it's probably clearer attached to each case individually, so I moved it.
scoder
pushed a commit
that referenced
this pull request
Dec 5, 2021
…H-4479) With reference to #4365 (comment) in Python 3 PyCode_NewEmpty does everything we need to set exception traceback code objects. This change is probably only a real improvement on Py3.11 onwards (where the replacement for PyCode_new is currently pretty slow). On earlier version it'll probably be fairly similar (maybe one extra allocation for the cline case?)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
With reference to #4365 (comment)
in Python 3 PyCode_NewEmpty does everything we need to set exception traceback code objects.
The downside is for the "cline" case, where it creates an extra unicode object, gets the
char*from that, and thenPyCode_NewEmptywill use that to create another unicode object (in principle this could probably mostly be avoided usingsnprintfor similar, but it doesn't seem worth it given that all these code objects are cached). For all other cases it should end up largely the same.It's mainly an improvement on the Py3.11 case (where the
PyCode_Newreplacement is likely fairly slow). For simplicity I've done it on all Python 3 versions, but it could also be done for Python 3.11 only.