API docs and pool usage (was: Re: svn commit: r14557 - in trunk/subversion: clients/cmdline include libsvn_client libsvn_diff svnlook)

["Peter N. Lundblad" <pe...@famlundblad.se>]

(I'm changing the subject line, since the API question below is more
important than the actual commit.)

On Fri, 6 May 2005, Philip Martin wrote:

> "Peter N. Lundblad" <pe...@famlundblad.se> writes:
>
> > On Wed, 4 May 2005, [UTF-8] Branko Ä^Libej wrote:
> >
> >> Philip Martin wrote:
> >>
> >> >Why the apr_pstrdup?
> >> >
> >> Yes, I just noticed this and asked myself the same thing. There's no
> >> need to copy this string around.
> >>
> > Because that's what the docstring says it does. So, why did he write the
> > docstring that way? Because I think it is consistent with what we do
> > elsewhere.
>
> I'm aware that input strings are sometimes duplicated before being
> returned, but that's to avoid making assumptions about the lifetime of
> the inputs.  That consideration doesn't apply in this case, so the
> consistency argument doesn't seem to apply.
>
We have lots of API routines that returns strings "allocated in @a pool".
So, I think we have two questions here:
- What should the docstrin gsay
and
- if the docstring says "allocated in @a pool", is the function allowed to
  return a statically allocated string?

For the first question, I think it is fine to allow the function to
allocate its return value out of @a pool. In general, returning statically
allocated strings is not a good idea IMO. Consider what happens if we want
to be able to change this value at runtime in the future? We could say
something like "Returns a statically allocated string, or a string
allocated in @a pool" if there was a compelling reason to do so. For cod
ehtat is used inside inner loops, I think this micro-optimization could be
useful. OK, you never know where your APIs will be used, but I can't
imagine that allocating memory out of a pool and copying a string,
typically shorter than 30 bytes, to be a bottleneck in any reasonable
scenario.  Philip, if you want the docstring to be changed, please say so
explicitly in your next reply.

For the second question, I think the answer is "no". Here's an example:

Say we have an application plugin (for whatever application you can think
of) that uses our libraries.  The app load the plugin that links to our
libs. The plugin calls an svn functiion that returns a value "allocated in
@a pool"; @a pool is controlled by the application. The plugin gives the
return value to the app, which it thinks is OK, since the alue is
allocated in a pool that the app controls. The app unloads the plugin and
then uses the data returned. Crash!

I can't say this scenario (or some variant thereof) is unreasonable and
therefore I think "allocated in @a pool" should really mean what it says.

> > Well, we could change it, but is it a big deal, really?
>
Presumably (answering my own question).

Regards,
//Peter

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: API docs and pool usage

["Peter N. Lundblad" <pe...@famlundblad.se>]

On Sat, 7 May 2005, [UTF-8] Branko �^Libej wrote:

> Peter N. Lundblad wrote:
>
> >Philip, if you want the docstring to be changed, please say so
> >explicitly in your next reply.
> >
> >
> I'm not Philip, but I suggest we change the docstring to "may be
> allocated in @a pool". We do that in other places where a function may
> return a statically or dynamically allocated object.
>
I'm fine with that change if anyone cares.

> >For the second question, I think the answer is "no". Here's an example:
> >
> >Say we have an application plugin (for whatever application you can think
> >of) that uses our libraries.  The app load the plugin that links to our
> >libs. The plugin calls an svn functiion that returns a value "allocated in
> >@a pool"; @a pool is controlled by the application. The plugin gives the
> >return value to the app, which it thinks is OK, since the alue is
> >allocated in a pool that the app controls. The app unloads the plugin and
> >then uses the data returned. Crash!
> >
> >
> Well, obvioiusly lifetime is important, and whatever you return must
> have a lifetime that is /at least/ the same as the lifetime of the pool,
> but there's nothing wrong with it being longer (and static strings have
> an "infinite" lifetime).

The lifetime is the same as the program's lifetime - in a world without
dynamic loading.

>
> >I can't say this scenario (or some variant thereof) is unreasonable and
> >therefore I think "allocated in @a pool" should really mean what it says.
> >
> >
> I agree with that. But I also think that adding a "may be" there is
> quite sufficient for our purposes. Certainly the function as it stands
> now should return the static pointer.
>
As I said, it is a very minimal optimization, but I don't object either.

Thanks,
//Peter

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: API docs and pool usage

[Branko Čibej <br...@xbc.nu>]

Greg Hudson wrote:

>I think you missed how Peter's scenario makes a static string have a
>shorter lifetime than the passed-in pool.
>  
>
Hm, you're right, I did miss that point.

-- Brane


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: API docs and pool usage

[Philip Martin <ph...@codematters.co.uk>]

Branko Čibej <br...@xbc.nu> writes:

> Philip Martin wrote:
>
>>In the end I don't really care, the allocation only happens on Windows
>>I don't use that :)
>>
> Oh, thanks a bundle... :\

I did show concern about TortoiseSVN performance in an email earlier
today :)

-- 
Philip Martin

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: API docs and pool usage

[Branko Čibej <br...@xbc.nu>]

Philip Martin wrote:

>In the end I don't really care, the allocation only happens on Windows
>I don't use that :)
>  
>
Oh, thanks a bundle... :\

-- Brane


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: API docs and pool usage

[Philip Martin <ph...@codematters.co.uk>]

Greg Hudson <gh...@MIT.EDU> writes:

> On Sat, 2005-05-07 at 13:39 +0200, Branko Èibej wrote:
>> >Say we have an application plugin (for whatever application you can think
>> >of) that uses our libraries.  The app load the plugin that links to our
>> >libs. The plugin calls an svn functiion that returns a value "allocated in
>> >@a pool"; @a pool is controlled by the application. The plugin gives the
>> >return value to the app, which it thinks is OK, since the alue is
>> >allocated in a pool that the app controls. The app unloads the plugin and
>> >then uses the data returned. Crash!
>
>> Well, obvioiusly lifetime is important, and whatever you return must 
>> have a lifetime that is /at least/ the same as the lifetime of the pool, 
>> but there's nothing wrong with it being longer (and static strings have 
>> an "infinite" lifetime).
>
> I think you missed how Peter's scenario makes a static string have a
> shorter lifetime than the passed-in pool.

I guess that's a valid concern in general, in this specific case the
static string is within svn_cmdline_init() and the documentation
states:

/** Set up the locale for character conversion, and initialize APR.
 * If @a error_stream is non-null, print error messages to the stream,
 * using @a progname as the program name. Return @c EXIT_SUCCESS if
 * successful, otherwise @c EXIT_FAILURE.
 *
 * @note This function should be called exactly once at program startup,
 *       before calling any other APR or Subversion functions.
 */

I suppose an application might attempt to access the memory after this
has unloaded, but it seems improbable.

In the end I don't really care, the allocation only happens on Windows
I don't use that :)

-- 
Philip Martin

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: API docs and pool usage

[Greg Hudson <gh...@MIT.EDU>]

On Sat, 2005-05-07 at 13:39 +0200, Branko Čibej wrote:
> I'm not Philip, but I suggest we change the docstring to "may be 
> allocated in @a pool". We do that in other places where a function may 
> return a statically or dynamically allocated object.

I object, based on Peter's dynamic loading scenario.  I think returning
a static string is generally a meaningless performance optimization, and
even the most bizarre of borderline cases should be considered a
showstopper bug for a meaningless optimization.

Apart from that, "the return value may be allocated in pool" is too
vague.  If it's not allocated in pool, there are a variety of other
options the implementation might use, such as a pool associated with one
of the passed-in objects.

> >Say we have an application plugin (for whatever application you can think
> >of) that uses our libraries.  The app load the plugin that links to our
> >libs. The plugin calls an svn functiion that returns a value "allocated in
> >@a pool"; @a pool is controlled by the application. The plugin gives the
> >return value to the app, which it thinks is OK, since the alue is
> >allocated in a pool that the app controls. The app unloads the plugin and
> >then uses the data returned. Crash!

> Well, obvioiusly lifetime is important, and whatever you return must 
> have a lifetime that is /at least/ the same as the lifetime of the pool, 
> but there's nothing wrong with it being longer (and static strings have 
> an "infinite" lifetime).

I think you missed how Peter's scenario makes a static string have a
shorter lifetime than the passed-in pool.


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: API docs and pool usage

[Branko Čibej <br...@xbc.nu>]

Peter N. Lundblad wrote:

>We have lots of API routines that returns strings "allocated in @a pool".
>So, I think we have two questions here:
>- What should the docstrin gsay
>and
>- if the docstring says "allocated in @a pool", is the function allowed to
>  return a statically allocated string?
>
>For the first question,
>
[snip]

>Philip, if you want the docstring to be changed, please say so
>explicitly in your next reply.
>  
>
I'm not Philip, but I suggest we change the docstring to "may be 
allocated in @a pool". We do that in other places where a function may 
return a statically or dynamically allocated object.

>For the second question, I think the answer is "no". Here's an example:
>
>Say we have an application plugin (for whatever application you can think
>of) that uses our libraries.  The app load the plugin that links to our
>libs. The plugin calls an svn functiion that returns a value "allocated in
>@a pool"; @a pool is controlled by the application. The plugin gives the
>return value to the app, which it thinks is OK, since the alue is
>allocated in a pool that the app controls. The app unloads the plugin and
>then uses the data returned. Crash!
>  
>
Well, obvioiusly lifetime is important, and whatever you return must 
have a lifetime that is /at least/ the same as the lifetime of the pool, 
but there's nothing wrong with it being longer (and static strings have 
an "infinite" lifetime).

>I can't say this scenario (or some variant thereof) is unreasonable and
>therefore I think "allocated in @a pool" should really mean what it says.
>  
>
I agree with that. But I also think that adding a "may be" there is 
quite sufficient for our purposes. Certainly the function as it stands 
now should return the static pointer.

-- Brane


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org