[Python-Dev] Making python C-API thread safe (try 2)

[Harri Pesonen]

Skip Montanaro wrote:

>    >> Actually, it makes perfect sense.  The reference count of Py_None is
>    >> never supposed to reach zero.  If that happens it's because you have
>    >> a bug (too many Py_DECREFs or not enouch Py_INCREFs).  Your version
>    >> of none_dealloc silently masks the error.
>
>    Harri> Yes, and there is no error if the reference count of Py_None
>    Harri> reaches zero. The Py_None reference count has no meaning.
>
>Yes it does.  If it goes to zero it means you have a bug in your code.  Note
>that there are plenty of situations where you INCREF or DECREF objects not
>knowing (or caring) that the actual object you are messing with might be
>Py_None.  If I happen to incompletely test a function defined (in C)
>something like:
>
>    def myfunc(arg1, arg2, arg3=None):
>        blah
>        blah
>        blah
>
>and have a reference count error in my code related to arg3 such that I
>erroneously DECREF it, but never test the three arg case, your version of
>none_dealloc() will silently miss the problem.  The current none_dealloc()
>will rightfully complain if myfunc() is called enough times, because
>Py_None's reference count will go to zero.
>
Yeah, but on the other hand, the application crashes less if it is not 
checked! :-)

>    Harri> By changing the way how Py_None is freed (by doing nothing)
>    Harri> Python would get simpler and faster.
>
>I think you're wrong.  There are lots of places in the current code base
>where the INCREFs and DECREFs happen without concern for the actual object
>passed.  There are at least some places where a check for Py_None would
>probably be warranted.  In any case, I suspect that most INCREFs and DECREFs
>of Py_None actually happen when the interpreter doesn't realize that Py_None
>is being manipulated.  By eliminating the few cases where you do know you're
>messing with Py_None you probably won't reduce the number of Py_INCREF and
>Py_DECREF calls substantially.
>
There are 2001 explicit calls to Py_INCREF(Py_None) in C API source 
code. These are unnecessary, but the implicit calls are of course fine. 
Removing the explicit calls would make Python slightly faster, just 
about enough to compensate the required changes in deallocation 
mechanism as least.

>    Harri> How can you have negative reference counts? Answer: You
>    Harri> can't.
>
>Yes you can.  From Include/object.h:
>
>    /* PyObject_HEAD defines the initial segment of every PyObject. */
>    #define PyObject_HEAD                       \
>            _PyObject_HEAD_EXTRA                \
>            int ob_refcnt;                      \
>            struct _typeobject *ob_type;
>
>Note that ob_refcnt is defined as an int, not an unsigned int.  I'm not sure
>if there are any ramifications to changing the type of that field.  I don't
>have time to inspect the code at the level necessary to answer
>authoritatively.
>
But if the reference count is negative for allocated objects, then you 
already have a bug. I mean that the count is never negative when the 
application works correctly. The object is already deallocated when the 
count reaches zero, so checking for negative counts is (usually) 
superfluous. Python gets slightly faster if you just remove this check 
(from release version).

>    Harri> I think that you are completely missing the point of Python's
>    Harri> reference counting. :-) The idea is that when the count reaches
>    Harri> zero, then the object is deallocated. But if the object was never
>    Harri> allocated in the first place, why deallocate it then? That's why
>    Harri> having empty none_dealloc is beautiful.
>
>Except for the case where the reference counting has an error.  Let me
>restate it this way: The C implementation of Python operationally defines a
>reference count of zero for a statically allocated object (not just Py_None)
>as a fatal error.  That's as it should be, because any statically allocated
>object should have a reference count of at least one.
>
Why? It has no meaning, because the object was never allocated, and it 
can't be deallocated. And if the deallocation routine does nothing for 
statically allocated objects (as it should), we are OK. I agree that 
there must be cases when you gain from this check (when you have bugs in 
your expansion code), but for pure Python source code this is just not 
needed.

And by the way, are are at least technically incorrect by saying "any 
statically allocated objects should have a reference count of at least 
one" because when Python starts, all these reference counts are already 
zero. The exception raises only when it reaches zero for the second 
time. :-)

>    Harri> I think that PyINCREF(Py_None) is ugly, and at least it is
>    Harri> completely unnecessary.
>
>You're the person who keeps asking for it.  Perhaps you should perform the
>tests.  If it works, submit a patch to SF.  If not, dig deeper and figure
>out why.
>  
>
Yeah, getting closer to that I guess. But I should not be doing it just 
yet, I have other projects, but this just keeps bugging me.

Harri