Return-Path: X-Original-To: apmail-subversion-dev-archive@minotaur.apache.org Delivered-To: apmail-subversion-dev-archive@minotaur.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id AAE2C1781F for ; Wed, 8 Oct 2014 08:26:11 +0000 (UTC) Received: (qmail 75952 invoked by uid 500); 8 Oct 2014 08:26:11 -0000 Delivered-To: apmail-subversion-dev-archive@subversion.apache.org Received: (qmail 75899 invoked by uid 500); 8 Oct 2014 08:26:11 -0000 Mailing-List: contact dev-help@subversion.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Delivered-To: mailing list dev@subversion.apache.org Received: (qmail 75889 invoked by uid 99); 8 Oct 2014 08:26:10 -0000 Received: from nike.apache.org (HELO nike.apache.org) (192.87.106.230) by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 08 Oct 2014 08:26:10 +0000 X-ASF-Spam-Status: No, hits=2.9 required=5.0 tests=HTML_MESSAGE,RCVD_IN_DNSWL_NONE,SPF_NEUTRAL X-Spam-Check-By: apache.org Received-SPF: neutral (nike.apache.org: local policy) Received: from [216.39.63.68] (HELO nm10-vm3.access.bullet.mail.gq1.yahoo.com) (216.39.63.68) by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 08 Oct 2014 08:25:41 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=att.net; s=s1024; t=1412756739; bh=aUfnDoHpjz+CDG3FklCg3+154xYaj+oZfpaSMQOTxRw=; h=Received:Received:Received:DKIM-Signature:X-Yahoo-Newman-Id:X-Yahoo-Newman-Property:X-YMail-OSG:X-Yahoo-SMTP:From:To:Cc:Subject:Date:Message-ID:User-Agent:In-Reply-To:References:MIME-Version:Content-Type:Content-Transfer-Encoding:From:Subject; b=b8IUGNXE6BmsC8ULkzMUP33YctiLb/M6585Jy6cHPdO5NeU0Pek+GK319WFjFOrxN0EQMiVpND4qfUAgXn78s7tqFKovqfRHly8f4d4wKV4qLa53gP+tD64gz5aGTC6te+4aNinas76zXOFeAfsE6Zx6LkdOo5rmmBm7uojZ3Q8= DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws; s=s1024; d=att.net; b=491ByVRFisrS+7J7VKu+niHgenfDxyxYhvfzx89LmkEqMaOQ9jiGWfdR/sRe0VOhbRexkBL/L2JSrSxS/RZI2NmiZ3rc6m/f5NKwO5LxrwMk2heIBV9xPnHX0pyfgyJe4GRQvWnai7id0sOx2BGtgjtT+AGpmK1uMQKur8W8dUc=; Received: from [216.39.60.172] by nm10.access.bullet.mail.gq1.yahoo.com with NNFMP; 08 Oct 2014 08:25:39 -0000 Received: from [67.195.23.146] by tm8.access.bullet.mail.gq1.yahoo.com with NNFMP; 08 Oct 2014 08:25:38 -0000 Received: from [127.0.0.1] by smtp118.sbc.mail.gq1.yahoo.com with NNFMP; 08 Oct 2014 08:25:38 -0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=att.net; s=s1024; t=1412756738; bh=aUfnDoHpjz+CDG3FklCg3+154xYaj+oZfpaSMQOTxRw=; h=X-Yahoo-Newman-Id:X-Yahoo-Newman-Property:X-YMail-OSG:X-Yahoo-SMTP:From:To:Cc:Subject:Date:Message-ID:User-Agent:In-Reply-To:References:MIME-Version:Content-Type:Content-Transfer-Encoding; b=ieDAyaeG0Wh+lPOTxl9le90B4AxCkLhrnlWLPTBnFR19yVlBL2ia0efYggVsSmlL2qf9qBO2lqpdf9u1iE9Im3F72A3Q9zrQJ1DjTB5+vrUkq7AGkPS+2qpCJzEi1ZsmiN6hQuSV8OzZ9HO/ipQbJTbExYMdX0jrSm3Uq0XOYMA= X-Yahoo-Newman-Id: 968628.78631.bm@smtp118.sbc.mail.gq1.yahoo.com X-Yahoo-Newman-Property: ymail-3 X-YMail-OSG: CVJ8CDsVM1mZRm1pKeUw2Phdhziqj8aHcPnOGQj1tpM9c6Y tZAOVa6rKmLvdfRZ.G_RmiJBdu5.HXoggyXmOz3nPGeeqDX5dLgwNRoc5NxL PhWCZj5l8wRZ5tf1fjSJ6byAAwR1NOHRsRjfL96pnST9spKlk4l9mKcLQwZQ Bes7tE2becoUij2tMkYltvp0V9ENRnWJ_4Ukst0qSGPaIWFhigSyeodO0HX_ B8xQXkdWmxfh6Ads5T7nP71bi61F_qGOG8Y34dtl1oiP0f9rxLi7H9_1KmNV PXBC3j5Z0EY8zKG3ZlMvz2K22NKrPsOFY9NQys8eEYVfD8D6PBnwjD9N5uQE aj8UI9vFvMgGUVx563fqquG3NZaP7oFVqCdxceboofc.c68yIOFdUFkgW_un TsTzy4War6O2dKOrC6H7OgRj65hwYOXtcPd7FPg61BEcrX8qZoB62NuVNjDj TJbBSa1lHwqsZibUVv93OYztMlnP2hrD4ejv.hZorHm.9LSyhzI6_sFG4aew T3AJu8cgQC69BjdQM21QLJZ9FQk0lUbFfB.s- X-Yahoo-SMTP: 0h0Q7euswBD_g.kcEqbzJWRFfrba801gq1M1 From: Alexey Neyman To: dev@subversion.apache.org Cc: Daniel Shahaf Subject: Re: Pool usage in Python bindings Date: Wed, 08 Oct 2014 01:25:37 -0700 Message-ID: <2053202.bv3jTtq7z4@mistral> User-Agent: KMail/4.13.3 (Linux/3.13.0-36-generic; KDE/4.13.3; x86_64; ; ) In-Reply-To: <20141008075650.GB1712@tarsus.local2> References: <2678554.1FMrfMabGH@mistral> <20141008075650.GB1712@tarsus.local2> MIME-Version: 1.0 Content-Type: multipart/alternative; boundary="nextPart9951748.OUmmatNrFg" Content-Transfer-Encoding: 7Bit X-Virus-Checked: Checked by ClamAV on apache.org This is a multi-part message in MIME format. --nextPart9951748.OUmmatNrFg Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" On Wednesday, October 08, 2014 07:56:50 AM Daniel Shahaf wrote: > Alexey Neyman wrote on Tue, Oct 07, 2014 at 17:27:09 -0700: > > I have encountered a problem with Subversion's Python bindings. For > > example, > > the following simple program does not work: > Which version of the bindings? trunk? > > > Am I correct in assuming that the division of a single 'pool' argument > > into distinct 'result_pool' and 'scratch_pool' is rather recent change > > and the bindings were not updated for that? > > The first instances are a few years old (1.6.0 svn_wc.h has dual pools). > New instances of dual pools happen when we create an entirely new public > API, or when we rev a single-pool API (i.e., generally, a pre-1.6 API), > in which case we make the revved API use dual pools. > > > Given all that, I could think of two approaches to fixing this issue. > > > > 1. Hackish, but simple. > > > > Change the call signature of the functions receiving one or two pool > > arguments: make them always receive a single argument for any pools > > it may have, which may be either None (meaning "use application pool"), > > a pool object (which will be used for both the result and scratch > > allocations) or a (result_pool, scratch_pool) tuple. > > > > This approach has two obvious drawbacks: first, it changes the > > number of arguments on the existing interfaces (but this may not > > be a bad thing, actually, given that it makes the breakage more > > explicit - rather than subtly using the scratch pool as described above). > > Second, it makes the argument lists for functions taking two pool > > arguments not match their C counterparts. E.g., svn_stream_open_readonly > > > > would be invoked as: > > // C > > res = svn_stream_open_readonly(&stream, "path", > > > > result_pool, scratch_pool); > > > > # Python > > stream = core.svn_stream_open_readonly("path", \ > > > > (result_pool, scratch_pool)) > > You said you had two approaches, but you only list one? > > Daniel > (I don't have time to digest your mail fully right now, although I get > the big picture.) Err, was the email truncated in your mailbox somehow? Right below the quoted piece was the following: Additionally, if there are functions that only take a scratch_pool argument, but still need a permanent pool for some allocations, they would need to be called with such tuple explicitly - at least with a (None, scratch_pool) tuple - even though the C counterpart only has a single pool argument. This is somewhat counterintuitive and error-prone, I think. 2. Less hackish, but requiring more effort. Do not use the passed-in pools for allocations in the wrapper itself. Instead, implement svn_swig_py_get_scratch_pool() and svn_swig_py_get_persistent_pool() - to create subpools of the application pool - and use these subpool in the typemaps for other types that need memory for conversion. The scratch pool will then be freed at the end of the wrapper. Persistent pools would originally be kept indefinitely. This is a leak, but it is probably better than what we do currently (using scratch pool for such allocations). Then, we could go over the interfaces using such persistent pools one by one, determine the required lifetime of such pool - record it in the Python object with the same lifetime and destroy the pool in that object's destructor. I am leaning towards the second approach. PS. I use svn_ra_session_t as an example where a semi-permanent allocation is necessary. However, I couldn't find the interface that destroys/closes the svn_ra_session_t. Is it correct that the svn_ra_session_t is always destroyed by destroying the corresponding session pool? If so, the bindings for this type need to extend it with a destructor function to call svn_pool_destroy(session->pool), or it will leak the memory if the application pool was used to create it. --nextPart9951748.OUmmatNrFg Content-Transfer-Encoding: 7Bit Content-Type: text/html; charset="us-ascii"

On Wednesday, October 08, 2014 07:56:50 AM Daniel Shahaf wrote:

> Alexey Neyman wrote on Tue, Oct 07, 2014 at 17:27:09 -0700:

> > I have encountered a problem with Subversion's Python bindings. For

> > example,

> > the following simple program does not work:

> Which version of the bindings? trunk?

>

> > Am I correct in assuming that the division of a single 'pool' argument

> > into distinct 'result_pool' and 'scratch_pool' is rather recent change

> > and the bindings were not updated for that?

>

> The first instances are a few years old (1.6.0 svn_wc.h has dual pools).

> New instances of dual pools happen when we create an entirely new public

> API, or when we rev a single-pool API (i.e., generally, a pre-1.6 API),

> in which case we make the revved API use dual pools.

>

> > Given all that, I could think of two approaches to fixing this issue.

> >

> > 1. Hackish, but simple.

> >

> > Change the call signature of the functions receiving one or two pool

> > arguments: make them always receive a single argument for any pools

> > it may have, which may be either None (meaning "use application pool"),

> > a pool object (which will be used for both the result and scratch

> > allocations) or a (result_pool, scratch_pool) tuple.

> >

> > This approach has two obvious drawbacks: first, it changes the

> > number of arguments on the existing interfaces (but this may not

> > be a bad thing, actually, given that it makes the breakage more

> > explicit - rather than subtly using the scratch pool as described above).

> > Second, it makes the argument lists for functions taking two pool

> > arguments not match their C counterparts. E.g., svn_stream_open_readonly

> >

> > would be invoked as:

> > // C

> > res = svn_stream_open_readonly(&stream, "path",

> >

> > result_pool, scratch_pool);

> >

> > # Python

> > stream = core.svn_stream_open_readonly("path", \

> >

> > (result_pool, scratch_pool))

>

> You said you had two approaches, but you only list one?

>

> Daniel

> (I don't have time to digest your mail fully right now, although I get

> the big picture.)

 

Err, was the email truncated in your mailbox somehow? Right below the quoted piece was the following:

 

Additionally, if there are functions that only take a scratch_pool

argument, but still need a permanent pool for some allocations, they

would need to be called with such tuple explicitly - at least with

a (None, scratch_pool) tuple - even though the C counterpart only

has a single pool argument. This is somewhat counterintuitive and

error-prone, I think.

2. Less hackish, but requiring more effort.

Do not use the passed-in pools for allocations in the wrapper

itself. Instead, implement svn_swig_py_get_scratch_pool() and

svn_swig_py_get_persistent_pool() - to create subpools of the

application pool - and use these subpool in the typemaps for other

types that need memory for conversion. The scratch pool will then

be freed at the end of the wrapper.

Persistent pools would originally be kept indefinitely. This is

a leak, but it is probably better than what we do currently

(using scratch pool for such allocations). Then, we could go

over the interfaces using such persistent pools one by one,

determine the required lifetime of such pool - record it in the

Python object with the same lifetime and destroy the pool in

that object's destructor.

I am leaning towards the second approach.

PS. I use svn_ra_session_t as an example where a semi-permanent

allocation is necessary. However, I couldn't find the interface

that destroys/closes the svn_ra_session_t. Is it correct that the

svn_ra_session_t is always destroyed by destroying the corresponding

session pool? If so, the bindings for this type need to extend it

with a destructor function to call svn_pool_destroy(session->pool),

or it will leak the memory if the application pool was used to

create it.

 

--nextPart9951748.OUmmatNrFg--