This is ready to review, the CI is finally green. Really no idea why the newly added GC tests are failing on Windows and unfortunately I don't have a Windows machine to debug this.

Looping in @carljm and @DinoV who have pointed out some of the issues with immortal instances in the permanent generation participating in a GC collection (i.e dicts). Let me know if you have some other thoughts or ideas on this!

Also looping in @vstinner. Finally got around upstreaming this patch since you recently wrote about this on your C-API Improvement Docs

My first reaction is that this shouldn't become part of the default build because most Python users will not make use of it and then it becomes pure extra overhead. However, I know for some people that it is a useful feature (e.g. pre-fork server architecture that exploits copy-on-write OS memory management). I would use it myself since I write web applications with that style.

Would it be okay to make this a compile time option, disabled by default? I think in general it is a bad idea to have too many of those types of build options. It makes code maintenance and testing more difficult. Some example build variations from the past that caused issues: thread/no-threads, Unicode width, various debug options (@vstinner removed some of those). So, I'm not super excited about introducing a new build option.

Is it possible we can leverage this extra status bit on objects to recover the lost performance somehow? A couple years ago I did a "tagged pointer" experiment that used a similar bit. In that case, small integers became one machine word in size and also become immortal.

Another thought: when you did your testing, were any objects made immortal? I would imagine that, by default, you could make everything immortal after initial interpreter startup. You are paying for an extra test+branch in INCREF and DECREF but for many objects (e.g. None, True, False, types) you avoid dirtying the memory/cache with writes to the reference count.

@nascheme you should definitely join the conversation happening in the bug report of this PR https://bugs.python.org/issue40255

However, I know for some people that it is a useful feature

Exactly, this change might be a feature for CPython power users

Would it be okay to make this a compile time option, disabled by default?

Yeah, that's probably the best option. That's also the consensus in the bug report thread (if the change is approved)

I think in general it is a bad idea to have too many of those types of build options.

Yeah that's one of the drawbacks. That being said, I can help with setting up the travis build to integrate this change if needed (cc @vstinner).

Is it possible we can leverage this extra status bit on objects to recover the lost performance somehow?

We can indeed, I think somebody also mentioned that in the bug report. A potentially good place could be main.c:pymain_main right after pymain_main. Let me explore that and push that change if it looks like performance a improvement!

In theory we could optimize even further to reduce the perf cost. By leveraging saturated adds and conditional moves we could remove the branching instruction. I haven't explored this further since the current PR was good enough. Personally, I favor the current PR, but this could be changed to:

/* Branch-less incref saturated at PY_SSIZE_T_MAX */
#define _Py_INC_REF(op) ({
    __asm__ (
        "addq $0x1, %[refcnt]"
        "cmovoq  %[refcnt_max], %[refcnt]"
        : [refcnt] "+r" (((PyObject *)op)->ob_refcnt)
        : [refcnt_max] "r" (PY_SSIZE_T_MAX)
    );})

/* Branch-less decref saturated at PY_SSIZE_T_MAX */
#define _Py_DEC_REF(op) ({
    Py_ssize_t tmp = ((PyObject *)op)->ob_refcnt;
    __asm__ (
        "subq $0x1, %[refcnt]"
        "addq $0x1, %[tmp]"
        "cmovoq  %[refcnt_max], %[refcnt]"
        : [refcnt] "+r" (((PyObject *)op)->ob_refcnt), [tmp] "+r" (tmp)
        : [refcnt_max] "r" (PY_SSIZE_T_MAX)
    );})

Yeah that's one of the drawbacks. That being said, I can help with setting up the travis build to integrate this change if needed (cc @vstinner).

Not only that, we would need specialized buildbots to test the code base with this option activated in a bunch of supported platforms and that raises the maintainance costs.

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@ericsnowcurrently Alright, I figured it out. It's a problem with C++ and more specifically std:c++20 which some builds use as an option. It has this clause where we cannot mix designated-initializers with non-designated-initializers.

The problem is that with the replacement of _PyObject_IMMORTAL_INIT with PyObject_HEAD_INIT we now introduced this issue where places like Include/internal/pycore_runtime_init.h are using designated initializers, whereas some other places in our codebase are not (like PC/_wmimodule.cpp or Lib/test/_testcppext.cpp).

To fix the "mix of designated and non-designated initializers" error in Include/internal/pycore_runtime_init.h, we now need to add designated initializers to PyObject_HEAD_INIT. However, if we do that, now Lib/test/_testcppext.cpp will have a "mix of designated and non-designated initializers" error. Now, the problem is that if we fix that and add designated initializers to these extensions, they now fail because C++ does not support designated initializers in non C++20.

I think the solution here is to either remove the designated initializers in Include/internal/pycore_runtime_init.h or, perhaps just create a _PyObject_HEAD_INIT that has designated in Include/internal/pycore_runtime_init.h. I'm inclining for the latter (and just include a comment about the C++20 errors), let me try it out and see how it goes.

Also cc @arhadthedev since it's related to what you wrote above.

@ericsnowcurrently I think it's working, in the meantime, would it be possible to revert 5c00a62 to make sure we get a clean build here?

Yeah, I'll look into that.

🤖 New build scheduled with the buildbot fleet by @ericsnowcurrently for commit a9caa2d 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

@ericsnowcurrently Alright, this seems to have fixed the issue designated initializers issue! Also, for some reason, a new warning came up with Mac/Clang build around "missing braces" and it causes test_cppext to fail. Looking into it already, should be easy to fix, you can see the sample error below. Separately, there are still failures with the broken tests on the main branch. We are super close now!

Error Sample:

./Modules/sha2module.c:792:5: warning: suggest braces around initialization of subobject [-Wmissing-braces]
    PyModuleDef_HEAD_INIT,
    ^~~~~~~~~~~~~~~~~~~~~
./Include/moduleobject.h:66:5: note: expanded from macro 'PyModuleDef_HEAD_INIT'
    PyObject_HEAD_INIT(_Py_NULL) \
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
./Include/object.h:134:9: note: expanded from macro 'PyObject_HEAD_INIT'
        _Py_IMMORTAL_REFCNT,     \
        ^~~~~~~~~~~~~~~~~~~
./Include/object.h:110:29: note: expanded from macro '_Py_IMMORTAL_REFCNT'
#define _Py_IMMORTAL_REFCNT UINT_MAX
                            ^~~~~~~~
/Library/Developer/CommandLineTools/usr/lib/clang/14.0.3/include/limits.h:56:19: note: expanded from macro 'UINT_MAX'
#define UINT_MAX  (__INT_MAX__  *2U +1U)
                  ^~~~~~~~~~~~~~~~~~~~~~

@ericsnowcurrently Fixed the Clang/MacOS problem. Given that we removed the designated initializers, the ob_refcnt type had issues with the clang compiler. Since we made the field a union, we now need braces to explicitly indicate that we are initializing the union. After adding them, the build was fixed and we should be back to green on the Mac/Clang builds.

Could you help me with a buildbot trigger? :)

🤖 New build scheduled with the buildbot fleet by @arhadthedev for commit 9cb2c21 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

@ericsnowcurrently I think we are good to go now. It seems like all the remaining test failures are due to test_urllib2, and test_urllib2net which are caused by 5c00a62.

Let me know how you would like to proceed!

It seems like all the remaining test failures are due to test_urllib2, and test_urllib2net which are caused by 5c00a62.

They are. The cause is urllib.request.__all__ not having HTTPSHandler:

The test failure is addressed in gh-103688.

Indeed, the module exports HTTPSHandler, though conditionally:

http.client.HTTPSConnection, in its turn, also exported conditionally:

I have no idea why some buildbots cannot import ssl thus failing the whole export chain.

congrats @eduardo-elizondo!!!

Thanks for all the hard work you put in and for being so very patient.

Three years in the making and it's finally there! Huge shoutouts to @ericsnowcurrently for working together with me throughout the years to get this all the way through, you rock!!

Also, thanks @markshannon @gvanrossum and @pablogsal for being a sounding board for ideas, reviews, and coaching on the messaging of the PR / PEP!

Congrats @eduardo-elizondo! 🔥

I followed this PR from the beginning, even needed to port the changes (in 2020) into python 3.9 due to the memory issue with multiprocessing for my previous company.

At some point, I felt this was not going to be in Python due to the inactivity here (especially the review).
It's incredible and amazing that you continued to finalize this work, and finally got it merged 🎉 !