You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Walter Mundt <em...@spamcop.net> on 2006/06/04 12:43:21 UTC
[PATCH] svn_client_ctx_t callback fixes, mark 3
Further refinement/discussion and the addition of some reference count
management code has resulted in another version of this patch.
[[[
Add necessary plumbing to Python bindings to allow the callbacks defined
in svn_client_ctx_t to function.
[ In subversion/bindings/swig ]
* include/proxy_apr.swg:
(apr_pool_t::_add_owned_ref, apr_pool_t::_remove_owned_ref): New
pool "owned reference" management functions.
(apr_pool_t::destroy): Now clears all "owned reference" items.
* python/libsvn_swig_py/swigutil_py.c:
(proxy_get_pool): New static function for getting the parent pool of
a proxy object. Logic extracted from svn_swig_MustGetPtr.
(svn_swig_py_pool_set_owned_ref): New helper function to manage pool
"owned references."
(svn_swig_MustGetPtr): Extracted proxy_get_pool function from here.
* include/svn_types.swg:
(PY_AS_VOID): New typemap for converting PyObjects into void* batons.
Uses svn_swig_py_pool_set_owned_ref helper to manage reference counts.
* svn_client.i:
Mark svn_client_ctx_t baton members as PY_AS_VOID
(svn_swig_py_cancel_func): new opaque pointer constant
(svn_swig_py_get_commit_log_func): new opaque pointer constant
(svn_swig_py_notify_func): new opaque pointer constant
* python/tests/client.py:
Fixed up doc comments throughout.
(SubversionRepositoryTestCase): Renamed test case class to
SubversionClientTestCase (original name is a probable typo)
(test_mkdir_url, test_log3_url): New tests.
(test_client_ctx_baton_lifetime): New test for PY_AS_VOID reference
counting semantics.
]]]
Third time's the charm? ;)
-Walter
Re: [PATCH] svn_client_ctx_t callback fixes, mark 3
Posted by Walter Mundt <em...@spamcop.net>.
Walter Mundt wrote:
> David James wrote:
>>> (svn_swig_py_pool_set_owned_ref): New helper function to manage pool
>>> "owned references."
>>
>> To quote the documentation: This function "replaces an 'owned
>> reference' allocated in a pool from oldRef to newRef. If oldRef is
>> non-NULL and present in the parent pool of proxy, it is removed."
>>
>> The replace operation in svn_swig_py_pool_set_owned_ref function is
>> atomic and thread-safe, assuming that the caller is holding the Python
>> global interpreter lock. (Is this a safe assumption to make?)
>
> I actually don't think it necessarily is. This function and several
> other helpers that are used in typemaps (most notably
> svn_swig_MustGetPtr) get called either before or after the block of code
> where the Python GIL is acquired in the generated code, but can execute
> arbitrary Python code. (At least here/on my version of swig.) In the
> case of proxy object getters and setters, the Python GIL is never
> acquired at all.
>
> While that seemed wrong to me, I decided not to worry about it for now,
> since the bindings currently do this and work anyway. Another patch for
> another day, assuming you don't pull another swig magic trick out of
> your hat that says the GIL is already acquired before calling some of
> these generated functions. (If you do that, then the code to
> acquire/release the GIL is inserted redundantly in a number of places.)
Just to clarify for the list, I had this completely backward in my head.
CPython extension functions own the Python GIL by default and must
release it if they don't want it. The bindings' current behavior is
correct, and djames's assumption above is safe after all.
-Walter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] svn_client_ctx_t callback fixes, mark 3
Posted by Walter Mundt <em...@spamcop.net>.
David James wrote:
>> (svn_swig_py_pool_set_owned_ref): New helper function to manage pool
>> "owned references."
>
> To quote the documentation: This function "replaces an 'owned
> reference' allocated in a pool from oldRef to newRef. If oldRef is
> non-NULL and present in the parent pool of proxy, it is removed."
>
> The replace operation in svn_swig_py_pool_set_owned_ref function is
> atomic and thread-safe, assuming that the caller is holding the Python
> global interpreter lock. (Is this a safe assumption to make?)
I actually don't think it necessarily is. This function and several
other helpers that are used in typemaps (most notably
svn_swig_MustGetPtr) get called either before or after the block of code
where the Python GIL is acquired in the generated code, but can execute
arbitrary Python code. (At least here/on my version of swig.) In the
case of proxy object getters and setters, the Python GIL is never
acquired at all.
While that seemed wrong to me, I decided not to worry about it for now,
since the bindings currently do this and work anyway. Another patch for
another day, assuming you don't pull another swig magic trick out of
your hat that says the GIL is already acquired before calling some of
these generated functions. (If you do that, then the code to
acquire/release the GIL is inserted redundantly in a number of places.)
> Oops. This should be "self.change_author". I fixed this before committing.
Sorry, forgot to fix this after the last patch!
> What do "obj0" and "arg1->$1_name" refer to? Why are these hacks
> necessary? It would be nice to follow up with a comment to explain
> what's going on here.
Sorry, I added this at the last minute and forgot to document it. obj0
is the Python proxy for the client context. arg1 is the actual client
context pointer. $1_name is a SWIG macro for the name of $input
(assuming 1 input), e.g. "log_msg_baton2". I needed access to both the
Python proxy object (for finding its parent pool) and the actual
previous value of the C field (for decrementing the pointed-to object's
refcount), and these were the only ways I could manage to get at both.
If you have less-hackish suggestions for either, they would be more than
welcome.
-Walter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] svn_client_ctx_t callback fixes, mark 3
Posted by David James <dj...@collab.net>.
On 6/4/06, Walter Mundt <em...@spamcop.net> wrote:
> Further refinement/discussion and the addition of some reference count
> management code has resulted in another version of this patch.
> [...]
> Third time's the charm? ;)
That's for sure! Oh man, this is a fantastic patch. I added a bunch of
extra documentation to explain what you did, and committed this in
r19930. Details below.
> [[[
> Add necessary plumbing to Python bindings to allow the callbacks defined
> in svn_client_ctx_t to function.
>
> [ In subversion/bindings/swig ]
>
> * include/proxy_apr.swg:
> (apr_pool_t::_add_owned_ref, apr_pool_t::_remove_owned_ref): New
> pool "owned reference" management functions.
> (apr_pool_t::destroy): Now clears all "owned reference" items.
These functions keep track of Python objects which are needed by this
pool. In this patch, we use memory pools to keep track of callback
functions and batons.
Here's an example of why this is necessary. When you type
"client_ctx.log_msg_baton2 = some_object", the PY_AS_VOID typemap
converts "some_object" into a simple pointer, and stores it inside the
client context as the log message baton. OK, that's great -- but how
do we ensure that the memory allocated by Python is still around when
the log message callback dereferences that pointer? That's where
"_add_owned_ref" comes in.
The PY_AS_VOID typemap marks "some_object" as owned by the same pool
as the client_ctx object. In other words, it uses _add_owned_ref to
add a reference to "some_object" from the pool. Therefore, as long as
the pool that owns "client_ctx" is around, "some_object" will be there
too. Therefore, we can be sure that some_object will be there when we
need it.
Some folks might ask: Why not just increment the reference count of
"some_object" in the PY_AS_VOID typemap?
Well sure, incrementing the reference count of "some_object" would
make sure that we keep it around, but then, unless we have a
corresponding call to decrement the reference count when the object is
no longer needed, we'll have a memory leak.
By simply adding a reference to "some_object" into the Python pool
object, we let Python keep track of the references. When Python kills
the pool, it'll kill the reference to "some_object" as well, therefore
freeing up the memory.
Great work! I added my explanation above into proxy_apr.swg so as to
help document the new behaviour.
> * python/libsvn_swig_py/swigutil_py.c:
> (proxy_get_pool): New static function for getting the parent pool of
> a proxy object. Logic extracted from svn_swig_MustGetPtr.
> (svn_swig_MustGetPtr): Extracted proxy_get_pool function from here.
Nice refactoring. This makes the code in swigutil_py.c easier to read.
> (svn_swig_py_pool_set_owned_ref): New helper function to manage pool
> "owned references."
To quote the documentation: This function "replaces an 'owned
reference' allocated in a pool from oldRef to newRef. If oldRef is
non-NULL and present in the parent pool of proxy, it is removed."
The replace operation in svn_swig_py_pool_set_owned_ref function is
atomic and thread-safe, assuming that the caller is holding the Python
global interpreter lock. (Is this a safe assumption to make?)
> * python/tests/client.py:
> Fixed up doc comments throughout.
> (SubversionRepositoryTestCase): Renamed test case class to
> SubversionClientTestCase (original name is a probable typo)
> (test_mkdir_url, test_log3_url): New tests.
> (test_client_ctx_baton_lifetime): New test for PY_AS_VOID reference
> counting semantics.
Nice tests! I added a few explanatory comments to the test case so as
to explain what you tested and why.
> + self.change_auther = None
Oops. This should be "self.change_author". I fixed this before committing.
> /* -----------------------------------------------------------------------
> + Mapper to automatically turn Python objects into void* batons on assignment
> +*/
> +
> +#ifdef SWIGPYTHON
> +%typemap(in) void *PY_AS_VOID (PyObject *newRef) {
> + newRef = $input;
> + if ($input == Py_None) {
> + $1 = newRef = NULL;
> + } else {
> + newRef = $input;
> + $1 = (void *)$input;
> + }
> + if (svn_swig_py_pool_set_owned_ref(obj0, (PyObject *)arg1->$1_name, newRef)) {
> + SWIG_fail;
> + }
> +}
> +#endif
What do "obj0" and "arg1->$1_name" refer to? Why are these hacks
necessary? It would be nice to follow up with a comment to explain
what's going on here.
Overall, great work, Walter! The extra work you took here to implement
completely correct memory management for the callback functions will
pay off big time in the long run.
Thanks,
David
--
David James -- http://www.cs.toronto.edu/~james
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org