Re: svn commit: r368769 - /apr/apr/trunk/include/apr_file_io.h

[Garrett Rooney <ro...@electricjellyfish.net>]

On 1/13/06, mturk@apache.org <mt...@apache.org> wrote:
> Author: mturk
> Date: Fri Jan 13 08:17:06 2006
> New Revision: 368769
>
> URL: http://svn.apache.org/viewcvs?rev=368769&view=rev
> Log:
> Mark pool param for apr_file_remove and apr_file_rename
> as unused (because it is).
> Perhaps some day it will be removed from the API.

I'm not sure how I feel about this.  The pool could potentially be
used for temporary allocation some day, if we port to a platform that
happens to require it.  Encouraging people to just pass random things
in place of the pool argument seems like a bad idea.

-garrett

Re: svn commit: r368769 - /apr/apr/trunk/include/apr_file_io.h

["William A. Rowe, Jr." <wr...@rowe-clan.net>]

Mladen Turk wrote:
> 
> The question is if they'll be ever used, or particularly,
> can someone write the implementation without using pool.

Contrawise, can someone write a better implementation, if given a pool?

> Since the return value from those functions is int, I'm
> almost sure they could be, by using stack for temp storage,

Huh?  How does the rv pertain to the implementation?

But on the issue of temp storage, win32 is VERY stack-heavy right now.
We all know stack is the least safe location to manipulate arrays.  Although
I trust (and trust that others have vetted) my utf8/ucs2 implementation, that
does not mean we shouldn't consider moving these hefty strings outside of the
stack, into a pool-based apr-specific filename cache or two (two, because some
ops like rename will require origin/destination pragmas).

If I did this, we would have to also point out that the pool passed to these
functions must be per-thread pools, or they could collide.  So it's not an
optimization I'd make until APR 2.

> and further more since no additional object is created that would
> require cleanup.

As others point out, Win32 is and isn't unique.  You destroy the ability for
authors of other obscure platforms to port apr when you clip their options
out from under them.  APR isn't 'done'.  At least, we hope it's not :)

> I think they were simply over engineered initially.

Again, perhaps, but the cost of passing a pool reference isn't measureable
against calling the filesystem to perform disk magic.

Bill

Re: svn commit: r368769 - /apr/apr/trunk/include/apr_file_io.h

[Garrett Rooney <ro...@electricjellyfish.net>]

On 1/13/06, Mladen Turk <mt...@apache.org> wrote:
> Garrett Rooney wrote:
> >>
> >>URL: http://svn.apache.org/viewcvs?rev=368769&view=rev
> >>Log:
> >>Mark pool param for apr_file_remove and apr_file_rename
> >>as unused (because it is).
> >>Perhaps some day it will be removed from the API.
> >
> > I'm not sure how I feel about this.  The pool could potentially be
> > used for temporary allocation some day, if we port to a platform that
> > happens to require it.  Encouraging people to just pass random things
> > in place of the pool argument seems like a bad idea.
> >
>
> Right, make sense in that case.
> There are also few other functions that have pool
> as a param but never use it like
> apr_env_delete or apr_dir_remove.
>
> The question is if they'll be ever used, or particularly,
> can someone write the implementation without using pool.
> Since the return value from those functions is int, I'm
> almost sure they could be, by using stack for temp storage,
> and further more since no additional object is created that would
> require cleanup.
>
> I think they were simply over engineered initially.

Sure, they can use stack space right up until they need to allocate
some memory that's dynamically sized, or call some other APR function
that needs a pool, or...  You see what I mean here.  The end result is
that when you take into account the fact that virtually any nontrivial
function in an APR application requires a pool anyway, we might as
well play it safe and design all but the most trivial APIs (and I
consider anything that isn't a totally in-memory operation on data
that's passed in to be non-trivial) so that they actually have access
to a pool.  You simply cannot predict the requirements of all
platforms, and it's better to have code written today work on those
platforms in the future when the cost (passing a pool) is trivial when
you consider that virtually all APIs in APR require it anyway.

We've actually had this sort of thing happen in Subversion, building
APIs that don't take pools and getting burned eventually by having to
create totally new APIs that do have them in order to accomplish what
was necessary.  It's just easier to have the pool from the beginning.

-garrett

Re: svn commit: r368769 - /apr/apr/trunk/include/apr_file_io.h

[Garrett Rooney <ro...@electricjellyfish.net>]

On 1/13/06, David Reid <da...@jetnet.co.uk> wrote:

> Adding the "unused" param is useful, but of course it's compiler
> specific how we code it, so we may need some autoconf foo to make it
> truly portable. That said, having the compiler know it's unused seems
> like a sensible goal to me rather than removing them.

I'm having difficulty envisioning a situation where a compiler
optimization resulting from the fact that the pool argument to the
remove and rename functions is unused could actually be noticable in
practice.

Regardless, that's not what Mladen's change did, he just updated the
documentation to indicate the you can pass anything you want for the
pool agument, which IMO can only hurt us if someday down the road we
do need to allocate memory in an implementation of these functions.

-garrett

Re: svn commit: r368769 - /apr/apr/trunk/include/apr_file_io.h

[David Reid <da...@jetnet.co.uk>]

Mladen Turk wrote:
> Garrett Rooney wrote:
> 
>>>
>>> URL: http://svn.apache.org/viewcvs?rev=368769&view=rev
>>> Log:
>>> Mark pool param for apr_file_remove and apr_file_rename
>>> as unused (because it is).
>>> Perhaps some day it will be removed from the API.
>>
>>
>> I'm not sure how I feel about this.  The pool could potentially be
>> used for temporary allocation some day, if we port to a platform that
>> happens to require it.  Encouraging people to just pass random things
>> in place of the pool argument seems like a bad idea.
>>
> 
> Right, make sense in that case.
> There are also few other functions that have pool
> as a param but never use it like
> apr_env_delete or apr_dir_remove.

Adding the "unused" param is useful, but of course it's compiler
specific how we code it, so we may need some autoconf foo to make it
truly portable. That said, having the compiler know it's unused seems
like a sensible goal to me rather than removing them.

> The question is if they'll be ever used, or particularly,
> can someone write the implementation without using pool.
> Since the return value from those functions is int, I'm
> almost sure they could be, by using stack for temp storage,
> and further more since no additional object is created that would
> require cleanup.
> 
> I think they were simply over engineered initially.

Perhaps.

> 
> Regards,
> Mladen.

Re: svn commit: r368769 - /apr/apr/trunk/include/apr_file_io.h

[Mladen Turk <mt...@apache.org>]

Garrett Rooney wrote:
>>
>>URL: http://svn.apache.org/viewcvs?rev=368769&view=rev
>>Log:
>>Mark pool param for apr_file_remove and apr_file_rename
>>as unused (because it is).
>>Perhaps some day it will be removed from the API.
> 
> I'm not sure how I feel about this.  The pool could potentially be
> used for temporary allocation some day, if we port to a platform that
> happens to require it.  Encouraging people to just pass random things
> in place of the pool argument seems like a bad idea.
>

Right, make sense in that case.
There are also few other functions that have pool
as a param but never use it like
apr_env_delete or apr_dir_remove.

The question is if they'll be ever used, or particularly,
can someone write the implementation without using pool.
Since the return value from those functions is int, I'm
almost sure they could be, by using stack for temp storage,
and further more since no additional object is created that would
require cleanup.

I think they were simply over engineered initially.

Regards,
Mladen.