I'm a confused by your description of approach 2:

Secretly check for tagged values inside API. That is -- make them support tagged values internally, but don't change their signature on the outside.

That does not look like the approach in the PR, which adds new functions that take tagged values. The approach in the PR seems correct. We can change the signatures of internal-only APIs as needed.

However, we still have to untag everything if we call escaping functions that call 3rd party code (e.g. vectorcall)

If you untag in-place on the stack, you need toPy_INCREF() deferred references: if you mark them as non-deferred on the evaluation stack then they need to reflect that.

I think it's better to do the untagging off the evaluation stack. In the common case, you can have some fixed sizePyObject *buf[N] on the C stack and untaginto it, for some reasonable small fixedN. In the general case,

I'm a confused by your description of approach 2:

Secretly check for tagged values inside API. That is -- make them support tagged values internally, but don't change their signature on the outside.

That does not look like the approach in the PR, which adds new functions that take tagged values. The approach in the PR seems correct for internal-only APIs -- we can change their signatures as needed.

I did a mix of both -- for internal functions I converted them to tagged form. For stuff that might be used by other people eg._PyList_FromArraySteal, I made them internally support tagged pointers.

I think it's better to do the untagging off the evaluation stack. In the common case, you can have some fixed sizePyObject *buf[N] on the C stack and untaginto it, for some reasonable small fixedN. In the general case,

Ok I will think about that. That does make sense -- to pass to a scratch buffer for now. If we exceed the buffer I assume we need to untag the C stack then. One thing we might want to be wary of is overflowing the C recursion stack with the current recursion limit. Since this will make_Py_EvalEvalFrameDefault a little larger.

Also to note: everyPy_DECREF(Py_CLEAR_TAG(x)) andPy_INCREF(Py_CLEAR_TAG(x)) and so on is an opportunity to swap out withPy_DECREF_TAGGED(x) and so on in a future PR, when we implement the actual deferring. To keep this PR small and easier to review, I didn't do them for now. Though I think I might as well since this PR is quite bulky already, and to reduce code churn in the future.

For stuff that might be used by other people eg. _PyList_FromArraySteal, I made them internally support tagged pointers.

That's undefined behavior in C -- let's avoid it if possible. If we're concerned about external users, add a new function that takes tagged references (and keep the old one unused, if desired).

If we exceed the buffer I assume we need to untag the C stack then...

If we exceed the fixed size buffer we should allocate enough space for a temporary buffer usingPyMem_Malloc().

One thing we might want to be wary of is overflowing the C recursion stack...

I would structure this so that the small, temporary buffer is only used when we perform a vectorcall (and not to a Python function). For example, in pseudo-code:

#defineN 5PyObject*PyObject_VectorcallTagged(PyObject*callable,const_Py_TaggedObject*tagged,size_tnargs,PyObject*kwnames){PyObject*args[N];if (nargs >=N) {returnPyObject_VectorcallTaggedSlow(callable,tagged,nargs,kwnames);  }untag(args,tagged,nargs);returnPyObject_Vectorcall(callable,args,nargs,kwnames);}PyObject*PyObject_VectorcallTaggedSlow(PyObject*callable,const_Py_TaggedObject*tagged,size_tnargs,PyObject*kwnames){PyObject*args=PyMem_Malloc(nargs*sizeof(PyObject*));if (args==NULL) ...untag(args,tagged,nargs);PyObject*res=PyObject_Vectorcall(callable,args,nargs,kwnames);PyMem_Free(args);returnres;}

Thanks for the clear explanation! I will address them tomorrow.

Hm, I now recall that the problem of untagging arrays of arguments sunk my attempt (I got it to work, but it was too slow). Hope you fare better!

Could you also configure pre-commit?https://devguide.python.org/getting-started/setup-building/#install-pre-commit

index 2bd9c40..3aa7dea 100644--- a/Tools/cases_generator/analyzer.py+++ b/Tools/cases_generator/analyzer.py@@ -359,7 +359,7 @@ def has_error_without_pop(op: parser.InstDef) -> bool:     "Py_XDECREF_STACKREF",     "Py_INCREF_STACKREF",     "Py_XINCREF_TAGGED",-    "Py_NewRef_StackRef",+    "Py_NewRef_StackRef",     "Py_INCREF",     "_PyManagedDictPointer_IsValues",     "_PyObject_GetManagedDict",

!buildbot nogil

🤖 New build scheduled with the buildbot fleet by@Fidget-Spinner for commitd59145b 🤖

The command will test the builders whose names match following regular expression:nogil

The builders matched are:

• x86-64 MacOS Intel ASAN NoGIL PR
• AMD64 Ubuntu NoGIL Refleaks PR
• ARM64 MacOS M1 NoGIL PR
• AMD64 Windows Server 2022 NoGIL PR
• AMD64 Ubuntu NoGIL PR
• ARM64 MacOS M1 Refleaks NoGIL PR
• x86-64 MacOS Intel NoGIL PR

Here are results from Sam'smicrobenchmark suite testing scalability. Note that I've only implemented deferred refcounting for method and function calls.

Specs:20 virtual cores (hyper threading)pyperf system tuneMain:object_cfunction MEGA FAILED: 1.1x slowercmodule_function MEGA FAILED: 2.1x slowergenerator FAILED: 3.1x fasterpymethod MEGA FAILED: 1.4x slowerpyfunction MEGA FAILED: 2.7x slowermodule_function MEGA FAILED: 1.9x slowerload_string_const MEGA FAILED: 2.1x slowerload_tuple_const MEGA FAILED: 1.9x slowercreate_closure MEGA FAILED: 3.6x slowercreate_pyobject MEGA FAILED: 2.9x slowerMy branch:object_cfunction FAILED: 9.3x fastercmodule_function MEGA FAILED: 2.0x slowergenerator FAILED: 2.9x fasterpymethod MEGA FAILED: 1.1x slowerpyfunction MEGA FAILED: 2.4x slowermodule_function MEGA FAILED: 1.9x slowerload_string_const MEGA FAILED: 2.2x slowerload_tuple_const MEGA FAILED: 2.1x slowercreate_closure MEGA FAILED: 3.6x slowercreate_pyobject MEGA FAILED: 3.1x slower

Notice thatobject_cfunction went from1.1x slower to9.3x faster on a 20 thread workload!

All tests pass except ASAN. I am now happy with the state this PR is in. So I will close this and start upstreaming things in peices.

Benchmarks for the default build suggest a 1% speeduphttps://github.com/faster-cpython/benchmarking-public/blob/main/results/bm-20240426-3.13.0a6%2B-d59145b/bm-20240426-linux-x86_64-Fidget%252dSpinner-tagged_evaluation_st-3.13.0a6%2B-d59145b-vs-base.md