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

Replace Py_INCREF()/Py_XINCREF() usage with Py_NewRef()/Py_XNewRef() #99300

Closed
vstinner opened this issue Nov 9, 2022 · 4 comments
Closed

Replace Py_INCREF()/Py_XINCREF() usage with Py_NewRef()/Py_XNewRef() #99300

vstinner opened this issue Nov 9, 2022 · 4 comments
Labels
type-bug An unexpected behavior, bug, or error

Comments

@vstinner
Copy link
Member

vstinner commented Nov 9, 2022

I added Py_NewRef() and Py_XNewRef() to Python 3.10 C API. IMO using them make to code easier to read and make the code looks "more correct". Examples:

(A) Assign + INCREF:

-                result = Py_False;
-                Py_INCREF(result);
+                result = Py_NewRef(Py_False);

(B) INCREF + assign:

-        Py_INCREF(last);
-        self->last = last;
+        self->last = Py_NewRef(last);

(C) INCREF + return:

-    Py_XINCREF(result);
-    return result;
+    return Py_XNewRef(result);

While technically, Py_INCREF() and Py_XINCREF() modify the object in-place (increment their reference counter), for me Py_NewRef() makes me sense: it creates "a new reference".

The example (A) is weird: it assigns a variable to something, and only later creates a new reference. For me, the syntax with Py_NewRef() makes more sense.

Examples (B) and (C) are shorter with Py_NewRef(), and again, IMO makes more sense and are more readable.

@vstinner vstinner added the type-bug An unexpected behavior, bug, or error label Nov 9, 2022
vstinner added a commit that referenced this issue Nov 10, 2022
Replace Py_INCREF() and Py_XINCREF() with Py_NewRef() and
Py_XNewRef() in C files of the Python/ directory.
vstinner added a commit that referenced this issue Nov 10, 2022
Replace Py_INCREF() and Py_XINCREF() with Py_NewRef() and
Py_XNewRef() in C files of the Python/ directory.

Update Parser/asdl_c.py to regenerate Python/Python-ast.c.
vstinner added a commit that referenced this issue Nov 10, 2022
Replace Py_INCREF() and Py_XINCREF() with Py_NewRef() and
Py_XNewRef() in Python/ceval.c and related files.
vstinner added a commit that referenced this issue Nov 10, 2022
Replace Py_INCREF() with Py_NewRef() in C files of the Parser/
directory and in the PEG generator.
vstinner added a commit that referenced this issue Nov 10, 2022
Replace Py_INCREF() and Py_XINCREF() with Py_NewRef() and
Py_XNewRef() in Objects/dictobject.c.
vstinner added a commit that referenced this issue Nov 10, 2022
Replace Py_INCREF() and Py_XINCREF() with Py_NewRef() and
Py_XNewRef() in C files of the Objects/ directory.
gvanrossum pushed a commit to gvanrossum/cpython that referenced this issue Nov 10, 2022
Replace Py_INCREF() and Py_XINCREF() with Py_NewRef() and
Py_XNewRef() in C files of the Objects/ directory.
gvanrossum pushed a commit to gvanrossum/cpython that referenced this issue Nov 10, 2022
Replace Py_INCREF() and Py_XINCREF() with Py_NewRef() and
Py_XNewRef() in Objects/dictobject.c.
vstinner added a commit that referenced this issue Nov 10, 2022
Replace Py_INCREF() and Py_XINCREF() with Py_NewRef() and
Py_XNewRef() in Objects/listobject.c.
vstinner added a commit that referenced this issue Nov 10, 2022
Replace Py_INCREF() and Py_XINCREF() with Py_NewRef() and
Py_XNewRef() in C files of the Objects/ directory.
vstinner added a commit that referenced this issue Nov 10, 2022
Replace Py_INCREF() and Py_XINCREF() with Py_NewRef() and
Py_XNewRef() in C files of the Objects/ directory.
vstinner added a commit that referenced this issue Nov 10, 2022
Replace Py_INCREF() and Py_XINCREF() with Py_NewRef() and
Py_XNewRef() in C files of the Objects/ directory.
ethanfurman pushed a commit to ethanfurman/cpython that referenced this issue Nov 12, 2022
Replace Py_INCREF() and Py_XINCREF() with Py_NewRef() and
Py_XNewRef() in C files of the Python/ directory.
ethanfurman pushed a commit to ethanfurman/cpython that referenced this issue Nov 12, 2022
Replace Py_INCREF() and Py_XINCREF() with Py_NewRef() and
Py_XNewRef() in C files of the Python/ directory.

Update Parser/asdl_c.py to regenerate Python/Python-ast.c.
ethanfurman pushed a commit to ethanfurman/cpython that referenced this issue Nov 12, 2022
Replace Py_INCREF() and Py_XINCREF() with Py_NewRef() and
Py_XNewRef() in Python/ceval.c and related files.
ethanfurman pushed a commit to ethanfurman/cpython that referenced this issue Nov 12, 2022
Replace Py_INCREF() with Py_NewRef() in C files of the Parser/
directory and in the PEG generator.
ethanfurman pushed a commit to ethanfurman/cpython that referenced this issue Nov 12, 2022
Replace Py_INCREF() and Py_XINCREF() with Py_NewRef() and
Py_XNewRef() in C files of the Objects/ directory.
ethanfurman pushed a commit to ethanfurman/cpython that referenced this issue Nov 12, 2022
Replace Py_INCREF() and Py_XINCREF() with Py_NewRef() and
Py_XNewRef() in Objects/dictobject.c.
@vstinner
Copy link
Member Author

I like the new idiom but not so much that I think it was worth all the code churn. (I would never have noticed you weren't doing this if it hadn't been for some merge conflicts in bytecodes.c, so you can ignore this comment. :-)

Sorry for the conflicts :-( I know that it's unpleasant. I tried to group changes related to ceval.c in a single commit to not require people have to fix conflicts multiple times: it's a single shot.

If you have troubles to fix your merge conflicts, I can help you to fix them. I have a tool to replace Py_INCREF with Py_NewRef().

Since I got positive feedbacks on other changes, I continue the work on this issue. The last big piece is the Modules/ directory. Then there are only a few remaining files which need to be changed.

FYI I started to collect some patterns which look suspicious and I plan to investigate them. It seems like I found a few reference leaks, and replacing Py_INCREF with Py_NewRef helped me to identify these.

I'm also considering to replace some "DECREF; INCREF; assign" patterns with Py_SETREF() when applicable. But I will open separated issues for these.

vstinner added a commit that referenced this issue Nov 14, 2022
Replace Py_INCREF() and Py_XINCREF() with Py_NewRef() and
Py_XNewRef() in test C files of the Modules/ directory.
vstinner added a commit that referenced this issue Nov 14, 2022
Replace Py_INCREF() and Py_XINCREF() with Py_NewRef() and
Py_XNewRef() in test C files of the Modules/ directory.
vstinner added a commit that referenced this issue Nov 14, 2022
Replace Py_INCREF() and Py_XINCREF() with Py_NewRef() and
Py_XNewRef() in Modules/_datetimemodule.c and Modules/_zoneinfo.c
vstinner added a commit that referenced this issue Nov 14, 2022
Replace Py_INCREF() and Py_XINCREF() with Py_NewRef() and
Py_XNewRef() in test C files of the Modules/ directory.
vstinner added a commit that referenced this issue Nov 14, 2022
Replace Py_INCREF() and Py_XINCREF() with Py_NewRef() and
Py_XNewRef() in test C files of the Modules/ directory.
vstinner added a commit that referenced this issue Nov 14, 2022
Replace Py_INCREF() and Py_XINCREF() with Py_NewRef() and
Py_XNewRef() in test C files of the Modules/ directory.
vstinner added a commit that referenced this issue Nov 14, 2022
Replace Py_INCREF() and Py_XINCREF() with Py_NewRef() and
Py_XNewRef() in test C files of the Doc/ directory.

Replace PyModule_AddObject() with PyModule_AddObjectRef() to simplify
reference counting.
vstinner added a commit that referenced this issue Nov 14, 2022
Replace Py_INCREF() and Py_XINCREF() with Py_NewRef() and
Py_XNewRef() in test C files of the PC/ directory.
@erlend-aasland
Copy link
Contributor

For the record, see gh-23170 for the PR that converted sqlite3 to Py_NewRef/Py_XNewRef.

vstinner added a commit that referenced this issue Nov 15, 2022
Replace Py_INCREF() and Py_XINCREF() with Py_NewRef() and
Py_XNewRef() in Python/Python-ast.c.

Update Parser/asdl_c.py to regenerate code.
CuriousLearner added a commit to CuriousLearner/cpython that referenced this issue Nov 16, 2022
* main: (8272 commits)
  Update Windows readme.txt to clarify Visual Studio required versions (pythonGH-99522)
  pythongh-99460 Emscripten trampolines on optimized METH_O and METH_NOARGS code paths (python#99461)
  pythongh-92647: [Enum] use final status to determine lookup or create (pythonGH-99500)
  pythongh-81057: Move Globals in Core Code to _PyRuntimeState (pythongh-99496)
  Post 3.12.0a2
  pythongh-99300: Use Py_NewRef() in Python/Python-ast.c (python#99499)
  pythongh-93649: Split pytime and datetime tests from _testcapimodule.c (python#99494)
  pythongh-99370: fix test_zippath_from_non_installed_posix (pythonGH-99483)
  pythonGH-99205: remove `_static` field from `PyThreadState` and `PyInterpreterState` (pythonGH-99385)
  pythongh-81057: Move the Remaining Import State Globals to _PyRuntimeState (pythongh-99488)
  pythongh-87604: Avoid publishing list of active per-interpreter audit hooks via the gc module (pythonGH-99373)
  pythongh-93649: Split getargs tests from _testcapimodule.c (python#99346)
  pythongh-81057: Move Global Variables Holding Objects to _PyRuntimeState. (pythongh-99487)
  pythonGH-98219: reduce sleep time in `asyncio` subprocess test (python#99464)
  pythonGH-99388: add `loop_factory` parameter to `asyncio.run` (python#99462)
  pythongh-99300: Use Py_NewRef() in PC/ directory (python#99479)
  pythongh-99300: Use Py_NewRef() in Doc/ directory  (python#99480)
  pythongh-99300: Use Py_NewRef() in Modules/ directory (python#99473)
  pythongh-99300: Use Py_NewRef() in Modules/ directory (python#99469)
  pythongh-99370: Calculate zip path from prefix when in a venv (pythonGH-99371)
  ...
vstinner added a commit that referenced this issue Nov 16, 2022
Replace Py_INCREF() and Py_XINCREF() using a cast with Py_NewRef()
and Py_XNewRef().
vstinner added a commit that referenced this issue Nov 16, 2022
Replace Py_INCREF() and Py_XINCREF() using a cast with Py_NewRef()
and Py_XNewRef().
@vstinner
Copy link
Member Author

vstinner commented Nov 21, 2022

26 commits later, I have converted all "INCREF + assign" and "INCREF + return" to NewRef. I am now closing the issue.

vstinner added a commit that referenced this issue Nov 22, 2022
)

* Replace Py_INCREF() and Py_XINCREF() using a cast with Py_NewRef()
  and Py_XNewRef() in Modules/_elementtree.c.
* Make reference counting more explicit: don't steal implicitly a
  reference on PyList_SET_ITEM(), use Py_NewRef() instead.
* Replace PyModule_AddObject() with PyModule_AddObjectRef().
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

3 participants