Uh oh!
There was an error while loading. Please reload this page.
gh-117139: Add header for tagged pointers - #118330
Conversation
colesbury
left a comment
There was a problem hiding this comment.
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 });
| 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.
Minor, but can we keep the _PyObject_HasDeferredRefcount and _PyObject_SetDeferredRefcount functions in pycore_object.h? We can #includepycore_object.h in pycore_tagged.h if needed.
- Keeps accessors for
ob_gc_bitsin the same file _PyStackRefuse is mostly limited to the interpreter and some supporting functions
There was a problem hiding this comment.
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.
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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Fidget-Spinner
commented
Apr 30, 2024
I adopted your naming scheme for all but the following:
|
Fidget-Spinner
commented
Apr 30, 2024
Note to self: when merging this, to add Sam as co-author. |
Co-Authored-By: Sam Gross <655866+colesbury@users.noreply.github.com>
colesbury
left a comment
There was a problem hiding this comment.
- 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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Co-Authored-By: Sam Gross <655866+colesbury@users.noreply.github.com>
colesbury
left a comment
There was a problem hiding this comment.
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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| #define PyStackRef_XSETREF(dst, src) \ | ||
| do { \ | ||
| _PyStackRef *_tmp_dst_ptr = &(dst) \ |
There was a problem hiding this comment.
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>
Uh oh!
There was an error while loading. Please reload this page.