GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-117139: Add header for tagged pointers by Fidget-Spinner · Pull Request #118330 · python/cpython

Conversation

@Fidget-Spinner

@Fidget-Spinner Fidget-Spinner commented Apr 26, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member
  • Issue: Set up tagged pointers in the evaluation stack #117139

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Fidget-Spinner added 2 commits April 27, 2024 02:37
@Fidget-Spinner
Add header for tagged pointers
209ddac
@Fidget-Spinner
cleanup
f783bc4
@bedevere-app bedevere-app Bot mentioned this pull request Apr 26, 2024
Set up tagged pointers in the evaluation stack #117139
Closed
@Fidget-Spinner Fidget-Spinner added the skip news label Apr 26, 2024
@Fidget-Spinner
end EOL
4c52acc
@Fidget-Spinner
Fidget-Spinner marked this pull request as ready for review April 27, 2024 03:25
@Fidget-Spinner
Fidget-Spinner requested review from a team, ericsnowcurrently and markshannon as code owners April 27, 2024 03:25
@bedevere-app bedevere-app Bot added the awaiting core review label Apr 27, 2024
@Fidget-Spinner
Fidget-Spinner requested review from colesbury and removed request for a team, ericsnowcurrently and markshannon April 27, 2024 03:25
Fidget-Spinner added 4 commits April 27, 2024 11:28
@Fidget-Spinner
fix warnings
9e0ed42
@Fidget-Spinner
format
4610cac
@Fidget-Spinner
make macro names more symmetrical
f0df2bc
@Fidget-Spinner
fix warning
35ac0ce
colesbury
colesbury reviewed Apr 29, 2024
View reviewed changes

@colesbury colesbury left a comment

Copy link
Copy Markdown
Contributor

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 });

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

zooba reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Include/internal/pycore_tagged.h Outdated
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
}

@colesbury colesbury Apr 29, 2024

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@Fidget-Spinner Fidget-Spinner Apr 30, 2024

Copy link
Copy Markdown
Member Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@ericsnowcurrently ericsnowcurrently Apr 30, 2024

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

colesbury and Fidget-Spinner reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
Comment thread Include/internal/pycore_tagged.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Include/internal/pycore_tagged.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Include/internal/pycore_tagged.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Include/internal/pycore_tagged.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Include/internal/pycore_tagged.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Include/internal/pycore_tagged.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

@Fidget-Spinner

Fidget-Spinner commented Apr 30, 2024

Copy link
Copy Markdown
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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Fidget-Spinner added 6 commits April 30, 2024 20:35
@Fidget-Spinner
address rest of review
c08a6f5
@Fidget-Spinner
remove MAX_UNTAG_SCRATCH
9b5f8b2
@Fidget-Spinner
rename file
046e308
@Fidget-Spinner
rename header guards
1a7087e
@Fidget-Spinner
add xnewref
2090980
@Fidget-Spinner
Add Py_STACKREF_NULL
2f50bcb
@Fidget-Spinner

Fidget-Spinner commented Apr 30, 2024

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Fidget-Spinner
fix Windows build
5961992
@Fidget-Spinner
Fidget-Spinner requested a review from colesbury April 30, 2024 13:09
@Fidget-Spinner @colesbury
try fix Windows for real this time
6bf6d44
Co-Authored-By: Sam Gross <[email protected]>
colesbury
colesbury reviewed Apr 30, 2024
View reviewed changes

@colesbury colesbury left a comment

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Include/internal/pycore_stackref.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Include/internal/pycore_stackref.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Include/internal/pycore_stackref.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Include/internal/pycore_stackref.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Include/internal/pycore_stackref.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Include/internal/pycore_stackref.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Include/internal/pycore_stackref.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Include/internal/pycore_stackref.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

Fidget-Spinner and others added 3 commits May 1, 2024 02:04
@Fidget-Spinner @colesbury
address review
003d274
Co-Authored-By: Sam Gross <[email protected]>
@Fidget-Spinner
revert hasdeferred refcount back to original file
7501357
@Fidget-Spinner
remove decref_owned
53608d7
colesbury
colesbury approved these changes Apr 30, 2024
View reviewed changes

@colesbury colesbury left a comment

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Include/internal/pycore_stackref.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Include/internal/pycore_stackref.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Include/internal/pycore_stackref.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting core review labels Apr 30, 2024
@Fidget-Spinner
address review
0131fc1
Fidget-Spinner
Fidget-Spinner commented Apr 30, 2024
View reviewed changes
Comment thread Include/internal/pycore_stackref.h

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

@Fidget-Spinner Fidget-Spinner Apr 30, 2024

Copy link
Copy Markdown
Member Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@Fidget-Spinner
Fidget-Spinner merged commit dc6b12d into python:main Apr 30, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label Apr 30, 2024
@Fidget-Spinner Fidget-Spinner added the topic-free-threading label May 1, 2024
@markshannon markshannon mentioned this pull request May 8, 2024
Deferred reference counts. faster-cpython/ideas#677
Open
SonicField pushed a commit to SonicField/cpython that referenced this pull request May 8, 2024
@Fidget-Spinner @colesbury @SonicField
pythongh-117139: Add header for tagged pointers (pythonGH-118330)
6585a6f
---------

Co-authored-by: Sam Gross <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ericsnowcurrently ericsnowcurrently ericsnowcurrently left review comments
@colesbury colesbury colesbury approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news topic-free-threading

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@Fidget-Spinner @colesbury @ericsnowcurrently
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.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.