Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

gh-127443: Update refcounts data with stable ABI functions.#127444

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Draft
picnixz wants to merge8 commits intopython:main
base:main
Choose a base branch
Loading
frompicnixz:doc/refcounts/update-127443

Conversation

picnixz
Copy link
Member

@picnixzpicnixz commentedNov 30, 2024•
edited
Loading

Since the changes were done manually and are mechanical, I'm pretty sure there are some +1 missing or incorrectly deduced. I've added some comments here and there about questions I had.

📚 Documentation preview 📚:https://cpython-previews--127444.org.readthedocs.build/

serhiy-storchaka, Eclips4, ZeroIntensity, and tomasr8 reacted with thumbs up emoji
@picnixzpicnixz added skip news docsDocumentation in the Doc dir labelsNov 30, 2024
@picnixzpicnixzforce-pushed thedoc/refcounts/update-127443 branch 6 times, most recently fromd1e6a24 tob798ba3CompareNovember 30, 2024 11:57
@picnixzpicnixzforce-pushed thedoc/refcounts/update-127443 branch fromb798ba3 toef9793dCompareNovember 30, 2024 12:07
@picnixzpicnixz marked this pull request as ready for reviewNovember 30, 2024 12:08
Copy link
Member

@serhiy-storchakaserhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Thank you for your work. I left some questions, but more thoughtful review I can only do in a week.

PyDict_GetItemRef:int:::
PyDict_GetItemRef:PyObject *:p:0:
PyDict_GetItemRef:PyObject *:key:0:
PyDict_GetItemRef:PyObject **:result:+1:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I am not sure how does it work with double pointers. Are there precedences?

BTW, are there any recirds with non-empty 5th field, and if there are any, what is its meaning?

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I am not sure how does it work with double pointers. Are there precedences?

One precedent is

PyErr_GetExcInfo:void:::PyErr_GetExcInfo:PyObject**:ptype:+1:PyErr_GetExcInfo:PyObject**:pvalue:+1:PyErr_GetExcInfo:PyObject**:ptraceback:+1:

I think the convention here is that we set the destination pointer to a new reference of the item we found, effectively increasing the reference count of the underlying object.

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

BTW, are there any recirds with non-empty 5th field, and if there are any, what is its meaning?

Yes, the meaning is just a "comment".

PyModule_Add:int:::
PyModule_Add:PyObject *:module:0:
PyModule_Add:const char *:name:0:
PyModule_Add:PyObject *:value:0:stolen

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Should not the refcount be -1?

Copy link
MemberAuthor

@picnixzpicnixzNov 30, 2024•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

There is a comment at the top of the file that says:

XXX NOTE: the 0/+1/-1 refcount information for arguments is
confusing! Much more useful would be to indicate whether the
function "steals" a reference to the argument or not. Take for
example PyList_SetItem(list, i, item). This lists as a 0 change for
both the list and the item arguments. However, in fact it steals a
reference to the item argument!

Technically, the reference count of the value itself does not change even though you're stealing the reference.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Tangentially related: can we add astolen comment to thePyList_SetItem? Though the comment at the beginning of the file explicitly says this, I still think it would be useful to add it.

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I think it would be good but I'd like to address the stealing references separately (easier for reviewers I think).

Eclips4 and encukou reacted with thumbs up emoji
@ZeroIntensity
Copy link
Member

ZeroIntensity commentedNov 30, 2024•
edited
Loading

Upon reviewing, I've discovered that I hate this format 😄. I'm going to throw together a script to convert this to TOML or something. Would others be interested in using such a script?

@picnixz
Copy link
MemberAuthor

Upon reviewing, I've discovered that I hate this format

I also don't like this format. We can convert it to TOML but what do you suggest?

@ZeroIntensity
Copy link
Member

TOML is probably the way to go here. Let's deal with converting later though.

Copy link
Member

@ZeroIntensityZeroIntensity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Some other things that I found that you didn't modify:

  • PyErr_NormalizeException is missing a reference count effect for a parameter.
  • So isPySeqIter_New.
  • PySequence_FAST_ITEMS is missing an effect for its return value (double check me on that, it returnsPyObject **).
  • PyUnicode_AsUTF8AndSize erroneously has a reference count effect for aPy_ssize_t * parameter.
  • Some functions that takePyModuleDef are missing reference count effects for the passed module def. Namely,PyModule_Create,PyModule_Create2,PyModule_ExecDef,PyModule_FromDefAndSpec,PyModule_FromDefAndSpec2,PyState_AddModule,PyState_FindModule,PyState_RemoveModule, andPyType_GetModuleByDef.

@@ -1878,6 +2234,26 @@ PyObject_TypeCheck:int:::
PyObject_TypeCheck:PyObject*:o:0:
PyObject_TypeCheck:PyTypeObject*:type:0:

PyObject_Vectorcall:PyObject *::+1:
PyObject_Vectorcall:PyObject *:callable:0:
PyObject_Vectorcall:PyObject *const *:args:0:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

We don't have a reference count effect for any of the otherPyObject *const * functions. Let's make a decision on whether to have it or not for these.

Suggested change
PyObject_Vectorcall:PyObject *const *:args:0:
PyObject_Vectorcall:PyObject *const *:args::

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I prefer to have them. We can think of args as being loosely typed asPyObject ** and since we are also managing double pointers like that, I think we can also manage those.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Could you fix the other cases then?

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I'd like to do it in a separate PR. I don't want to change existing lines in this specific PR.

ZeroIntensity and encukou reacted with thumbs up emoji
PyUnicodeTranslateError_SetStart:int:::
PyUnicodeTranslateError_SetStart:PyObject *:exc:0:
PyUnicodeTranslateError_SetStart:Py_ssize_t:start::

PyWeakref_Check:int:::
PyWeakref_Check:PyObject*:ob::
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This, as well asPyWeakref_CheckRef andPyWeakref_CheckProxy below it, need reference count effects.

Suggested change
PyWeakref_Check:PyObject*:ob::
PyWeakref_Check:PyObject*:ob:0:

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Addressed in#127451.

@picnixz
Copy link
MemberAuthor

Some of those are done in the other PR! I'll address the remaining in a few hours . Thanks!

In PyErr_SetHandledException, the exception's reference count is incremented by 1 since it's being used as `Py_XSETREF(..., Py_NewRef(exc))`.In PyType_FromModuleAndSpec, the module's reference count is incremented by 1 since it's being stored in the new class being created.
PyOS_string_to_double:double:::
PyOS_string_to_double:const char *:s::
PyOS_string_to_double:char **:endptr::
PyOS_string_to_double:PyObject *:overflow_exception:+1:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Is +1 correct here? It's ±0 on success.

@@ -3016,10 +3812,50 @@ _PyBytes_Resize:int:::
_PyBytes_Resize:PyObject**:bytes:0:
_PyBytes_Resize:Py_ssize_t:newsize::

_PyObject_CallFunction_SizeT:PyObject *::+1:abi-only
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Please pick only one ofabi_only orabi-only.

@picnixzpicnixz marked this pull request as draftMarch 31, 2025 09:33
@picnixz
Copy link
MemberAuthor

picnixz commentedMar 31, 2025•
edited
Loading

Marking it asstale because I still didn't have time to work on the project separately (I'm working on it but I'm advancing slowly) [I don't want to choose a new grammar here if later we're changing it...]

@picnixzpicnixz added the staleStale PR or inactive for long period of time. labelMar 31, 2025
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@encukouencukouencukou left review comments

@serhiy-storchakaserhiy-storchakaserhiy-storchaka left review comments

@ZeroIntensityZeroIntensityZeroIntensity left review comments

@Eclips4Eclips4Awaiting requested review from Eclips4

Assignees
No one assigned
Labels
docsDocumentation in the Doc dirskip newsstaleStale PR or inactive for long period of time.
Projects
Status: Todo
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

5 participants
@picnixz@ZeroIntensity@encukou@serhiy-storchaka@Eclips4
[8]ページ先頭
©2009-2025 Movatter.jp