GeezerVison

Is this necessary?
Doesn't the profiler extract the same data from the builtin function that it could from the method descriptor?

This piece is copied from the new setprofile I believe. The idea behind it is to make sure get_cfunc_from_callable only returns a PyCFunctionObject. If it's not, then ((PyCFunctionObject *)cfunc)->m_ml won't work.

What do you propose here? Simply return callable? We need to check that anyway because CALL event can be triggered before calling a Python function and we don't want to add profiler entry on that(it should be dealt with later). I did realize that Py_RETURN_NONE was incorrect - NULL should be returned.

All that happens to the builtin function objects is it gets passed down to normalizeUserObj() which then does some elaborate lookup to get the method descriptor back again. Both the method descriptor and builtin function contain a pointer to the same PyMethodDef struct.

So, leave the method descriptor alone here, and in normalizeUserObj() create the same string that as would be created for the builtin function.

The mo object on line 175 is the method descriptor.

There are three purposes get_cfunc_from_callable needs the serve:

1. To get the object(callable) to print later, which could be resolved in normalizeUserObj() like you said.
2. To get a unique key for the hash table(actually a binary tree). In this case, ((PyCFunctionObject *)cfunc)->m_ml was used, maybe the callable itself could be used as well? Not sure why m_ml was chosen at the first place, the builtin functions probably have dinstinct addresses anyway? Even for the builtin methods(like list.append), using (void*)callable directly might work?
3. To filter out unwanted data. This is why this part is necessary. Without trying to get the C function from the method, how could we know if this is a call to a builtin function that we would like to log? It could be a simple Python call(f()), which would trigger CALL event. It could be a method call, but not to a C function(self.some_method()). We need to filter these entry out before we even reach normalizeUserObj()(that's where we are about to log the function call).

The current implementation is trying to swap out the setprofile layer without touching the internal profiling system. It's true that the profiling system could be optimized, but it's also risky and probably need extra care. If we want to land this in 3.12, maybe we should avoid changing the profiling logic for now.

I agree with your point about risk.

I think it is worth cleaning up the internals. We couldn't do it before, as the conversion from method descriptor to builtin function occurred before cprofile got to see it. Maybe we can get it done for 3.12, maybe not.

So let's get this change in for 3.12, and we can streamline things later.

We can clean up the internals, but we still need to address point 3. We need to filter out entries that we don't want, and that probably requires resolving the descriptors.

All the information in the fake builtin function is also in the original method descriptor, so whatever the filter was doing should still work.

Do you suggest that we can get rid of Py_TYPE(callable)->tp_descr_get(callable, self_arg, (PyObject*)Py_TYPE(self_arg)) because we don't need the actual PyCFunctionObject in it? We still need to check against Py_TYPE(callable) == &PyMethodDescr_Type for the actual "builtin methods" right? To filter out Python defined methods?

Why has this line been removed?

Because what the test was partially testing won't work anymore. The line relies on the fact that the original obj was deallocated when replace - that won't happen because it's still being referenced by the monitors.

Having that line would also cause an infinite exception loop, probably due to the new mechanism.

Sorry not infinite, but very frequent warning. The issue is we did not disable the profiling - that's what I mentioned in the description - enabling the profiling with one object and disable it with another won't work.