-
-
Notifications
You must be signed in to change notification settings - Fork 31.2k
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
gh-117139: Add header for tagged pointers #118330
Conversation
Fidget-Spinner
commented
Apr 26, 2024
•
edited
Loading
edited
- Issue: Set up tagged pointers in the evaluation stack #117139
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The names of some of the functions could use some work:
Py_STACKREF_TAG
Py_STACKREF_TAG_DEFERRED
Py_STACKREF_UNTAG_BORROWED
Py_STACKREF_UNTAG_OWNED
Maybe the following:
// instead of Py_STACKREF_TAG
// Converts a PyObject * to a PyStackRef, stealing the reference
PyStackRef_StealRef(PyObject*) -> PyStackRef
// instead of Py_NewRef_StackRef(Py_STACKREF_TAG_DEFERRED(...));
// Converts a PyObject * to a PyStackRef, with a new reference
PyStackRef_NewRef(PyObject*) -> PyStackRef
// instead of Py_STACKREF_UNTAG_BORROWED
PyStackRef_Get(PyStackRef) -> PyObject *
// or maybe
PyStackRef_GET(PyStackRef) -> PyObject *
// instead of Py_STACKREF_UNTAG_OWNED
PyStackRef_StealObject -> PyObject *
I don't think we should have a function that only does what Py_STACKREF_TAG_DEFERRED
currently does -- instead oI think we should have a function that effectively does the combo Py_NewRef_StackRef(Py_STACKREF_TAG_DEFERRED())
but more efficiently.
Additionally, instead of Py_STACKREF_TAG(NULL)
let's have a macro for the constant, i.e. something like #define Py_STACKREF_NULL ({_PyStackRef) { .bits = 0 });
Include/internal/pycore_tagged.h
Outdated
static inline int | ||
_PyObject_HasDeferredRefcount(PyObject *op) | ||
{ | ||
#ifdef Py_GIL_DISABLED | ||
return (op->ob_gc_bits & _PyGC_BITS_DEFERRED) != 0; | ||
#else | ||
return 0; | ||
#endif | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor, but can we keep the _PyObject_HasDeferredRefcount
and _PyObject_SetDeferredRefcount
functions in pycore_object.h
? We can #include
pycore_object.h
in pycore_tagged.h
if needed.
- Keeps accessors for
ob_gc_bits
in the same file _PyStackRef
use is mostly limited to the interpreter and some supporting functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried this in the original branch but we can't easily for some reason. We can't #include pycore_object.h
in pycore_tagged.h
because it causes anything that includes pycore_interp.h
to fail to compile. Not sure why.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, my old enemy...circular includes. When this has happened to me in the past, I solved this by splitting out the problematic parts of the header file into a new "pycore_..._state.h". It holds just the structs needed by pycore_interp.h. Then pycore_interp.h includes only the "_state.h" file. You can see examples in Include/internal.
I adopted your naming scheme for all but the following:
|
Note to self: when merging this, to add Sam as co-author. |
Co-Authored-By: Sam Gross <655866+colesbury@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Would you please clean-up the formatting of the functions? I gave one example of function below.
- Let's revert the move of
_PyObject_SetDeferredRefcount
. From my local testing, it doesn't look like it's necessary in this PR. If we run into circular imports, we can do a better refactoring as they come up with @ericsnowcurrently's suggestion.
Co-Authored-By: Sam Gross <655866+colesbury@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more minor comments, but otherwise LGTM.
I'm still not thrilled with the names we have (I know, I suggested a bunch of them...), but given that it's an internal-only header we can update them later if we come up with something better.
Right now we have four conversion functions:
PyObject *PyStackRef_Get(_PyStackRef)
PyObject *PyStackRef_StealObject(_PyStackRef)
_PyStackRef PyStackRef_StealRef(PyObject *)
_PyStackRef PyStackRef_NewRefDeferred(PyObject *)
There's a lot of similarity between them, but I don't think the naming conventions currently do a good job of sigifying that.
|
||
#define PyStackRef_XSETREF(dst, src) \ | ||
do { \ | ||
_PyStackRef *_tmp_dst_ptr = &(dst) \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a missing semicolon here but I don't want to waste CI resources, so I will fix this in another PR, since nothing is using this file at the moment anyways.
--------- Co-authored-by: Sam Gross <655866+colesbury@users.noreply.github.com>