You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Davi Arnaut <da...@haxent.com.br> on 2007/07/11 20:59:22 UTC
apr_pools hairsplitting
Hi,
The current apr_pools.c file is a serious case of "spaghetti" code
(ifdefs, debug and non-debug mixed, etc), making it hard to read f not
impossible (subtle changes, catastrophic impact).
In order to increase the overall readability and foster future
improvements, I've separated the apr_pools.c file into more sane and
self contained files:
apr-pools-allocator.patch # allocator.c apr_allocator
apr-pools-print.patch # print.c apr_psprintf and family
apr-pools-cleanup.patch # cleanup.c pool cleanup functions
apr-pools-properties.patch # propertiees.c set/get methods
apr-pools-debug.patch # pools_debug.c pool debug code
apr-pools-consolidate.patch # pools.c pool non-debug code
Patchset available at http://people.apache.org/~davi/pools/
Before:
text data bss dec hex filename
164467 3664 232 168363 291ab libapr-1.so.0.3.0
apr_pools.c 2333 lines
libapr-1.so.0.3.0 173K
After:
text data bss dec hex filename
162033 3680 232 165945 28839 libapr-1.so.0.3.0
pools.c 434 lines
libapr-1.so.0.3.0 169K
Comments?
--
Davi Arnaut
Re: apr_pools hairsplitting
Posted by Sander Striker <s....@striker.nl>.
On 7/11/07, Davi Arnaut <da...@haxent.com.br> wrote:
> Hi,
>
> The current apr_pools.c file is a serious case of "spaghetti" code
> (ifdefs, debug and non-debug mixed, etc), making it hard to read f not
> impossible (subtle changes, catastrophic impact).
:-) Have a look through the history of that file, just for fun :-D.
> In order to increase the overall readability and foster future
> improvements, I've separated the apr_pools.c file into more sane and
> self contained files:
My $0.02 is that comments would go a long way; it's the department where
I have been lacking. Together with the apr_pool_free() function that never
actually made it into the codebase...
[...]
> Comments?
Splitting allocators and pools is very likely going to be a bad idea because
of inlining. Other functionality you can likely split out. I'm a bit
wary about
splitting out psprintf as that too hooks in very tightly with the allocation
logic.
Be very careful with changes, as the performance impact can be quite vast.
While we were working on this code, Brian Pane and others were doing
comparison tests between builds.
Cheers,
Sander
Re: apr_pools hairsplitting
Posted by Davi Arnaut <da...@haxent.com.br>.
Justin Erenkrantz wrote:
> On 7/17/07, Davi Arnaut <da...@haxent.com.br> wrote:
>>> Have you looked at where the text size reduction comes from? Fewer
>>> static functions being inlined?
>> mostly from the uninline of allocator_alloc and allocator_free.
>
> Since a lot of performance tuning was done on the allocator and pools,
> does this change and, in particular, removing the inlining, have any
> noticeable impact on our downstream apps - such as httpd or SVN?
>
It might make it slightly faster because of the reduce cache footprint
and reduced register pressure, but measuring the overall impact is
harder. Inlining is usually only advantageous for very small functions,
allocator_alloc only bloats the callers even more.
Inline expansions:
name totsz / exp# = avgsz src#
allocator_free 710 4l 177 1
allocator_alloc 1596 4l 399 1
Call sites:
allocator_alloc: apr_palloc [1], apr_pool_create_ex, psprintf_flush [1]
allocator_free: apr_pool_clear
[1] Only if a free node is not available.
--
Davi Arnaut
Re: apr_pools hairsplitting
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
On 7/17/07, Davi Arnaut <da...@haxent.com.br> wrote:
> > Have you looked at where the text size reduction comes from? Fewer
> > static functions being inlined?
>
> mostly from the uninline of allocator_alloc and allocator_free.
Since a lot of performance tuning was done on the allocator and pools,
does this change and, in particular, removing the inlining, have any
noticeable impact on our downstream apps - such as httpd or SVN?
Thanks. -- justin
Re: apr_pools hairsplitting
Posted by Davi Arnaut <da...@haxent.com.br>.
Joe Orton wrote:
> On Wed, Jul 11, 2007 at 03:59:22PM -0300, Davi Arnaut wrote:
>> Before:
>>
>> text data bss dec hex filename
>> 164467 3664 232 168363 291ab libapr-1.so.0.3.0
> ...
>> After:
>>
>> text data bss dec hex filename
>> 162033 3680 232 165945 28839 libapr-1.so.0.3.0
>
> Have you looked at where the text size reduction comes from? Fewer
> static functions being inlined?
mostly from the uninline of allocator_alloc and allocator_free.
> In principle this looks good, though it's annoying that it forces a
> bunch of stuff to be global rather than static.
I was planning to use the gcc visibility support to hide those symbols
(and others internal-only). I'm just not sure if we should create a new
APR_INTERNAL(type) with "hidden" visibility or change APR_DECLARE(type)
to a "default" visibility and compile apr with -fvisibility=hidden.
--
Davi Arnaut
Re: apr_pools hairsplitting
Posted by Joe Orton <jo...@redhat.com>.
On Wed, Jul 11, 2007 at 03:59:22PM -0300, Davi Arnaut wrote:
> Before:
>
> text data bss dec hex filename
> 164467 3664 232 168363 291ab libapr-1.so.0.3.0
...
> After:
>
> text data bss dec hex filename
> 162033 3680 232 165945 28839 libapr-1.so.0.3.0
Have you looked at where the text size reduction comes from? Fewer
static functions being inlined?
In principle this looks good, though it's annoying that it forces a
bunch of stuff to be global rather than static.
joe