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

gh-117139: Add header for tagged pointers #118330

Merged
merged 19 commits into from Apr 30, 2024

Conversation

Fidget-Spinner
Copy link
Member

@Fidget-Spinner Fidget-Spinner commented Apr 26, 2024

@Fidget-Spinner Fidget-Spinner marked this pull request as ready for review April 27, 2024 03:25
@Fidget-Spinner Fidget-Spinner requested review from colesbury and removed request for a team, ericsnowcurrently and markshannon April 27, 2024 03:25
Copy link
Contributor

@colesbury colesbury left a 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 });

Comment on lines 18 to 26
static inline int
_PyObject_HasDeferredRefcount(PyObject *op)
{
#ifdef Py_GIL_DISABLED
return (op->ob_gc_bits & _PyGC_BITS_DEFERRED) != 0;
#else
return 0;
#endif
}
Copy link
Contributor

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

Copy link
Member Author

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.

Copy link
Member

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.

Include/internal/pycore_tagged.h Outdated Show resolved Hide resolved
Include/internal/pycore_tagged.h Outdated Show resolved Hide resolved
Include/internal/pycore_tagged.h Outdated Show resolved Hide resolved
Include/internal/pycore_tagged.h Outdated Show resolved Hide resolved
Include/internal/pycore_tagged.h Outdated Show resolved Hide resolved
Include/internal/pycore_tagged.h Outdated Show resolved Hide resolved
@Fidget-Spinner
Copy link
Member Author

I adopted your naming scheme for all but the following:

PyStackRef_NewRef
This is too close to the actual equivalent of PyStackRef_NewRef(PyStackRef_StealRef(obj)). To distinguish it, I will call it PyStackRef_NewRefDeferred instead.

@Fidget-Spinner
Copy link
Member Author

Note to self: when merging this, to add Sam as co-author.

Co-Authored-By: Sam Gross <655866+colesbury@users.noreply.github.com>
Copy link
Contributor

@colesbury colesbury left a 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.

Include/internal/pycore_stackref.h Outdated Show resolved Hide resolved
Include/internal/pycore_stackref.h Outdated Show resolved Hide resolved
Include/internal/pycore_stackref.h Outdated Show resolved Hide resolved
Include/internal/pycore_stackref.h Outdated Show resolved Hide resolved
Include/internal/pycore_stackref.h Outdated Show resolved Hide resolved
Include/internal/pycore_stackref.h Outdated Show resolved Hide resolved
Include/internal/pycore_stackref.h Outdated Show resolved Hide resolved
Include/internal/pycore_stackref.h Outdated Show resolved Hide resolved
Fidget-Spinner and others added 3 commits May 1, 2024 02:04
Co-Authored-By: Sam Gross <655866+colesbury@users.noreply.github.com>
Copy link
Contributor

@colesbury colesbury left a 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.

Include/internal/pycore_stackref.h Outdated Show resolved Hide resolved
Include/internal/pycore_stackref.h Outdated Show resolved Hide resolved
Include/internal/pycore_stackref.h Outdated Show resolved Hide resolved

#define PyStackRef_XSETREF(dst, src) \
do { \
_PyStackRef *_tmp_dst_ptr = &(dst) \
Copy link
Member Author

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.

@Fidget-Spinner Fidget-Spinner merged commit dc6b12d into python:main Apr 30, 2024
34 checks passed
SonicField pushed a commit to SonicField/cpython that referenced this pull request May 8, 2024

---------

Co-authored-by: Sam Gross <655866+colesbury@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants