You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Ivan Zhakov <iv...@visualsvn.com> on 2017/03/27 13:55:16 UTC
Re: svn commit: r1788334 - in /apr/apr/trunk: CHANGES
include/apr_allocator.h memory/unix/apr_pools.c
On 24 March 2017 at 00:34, <yl...@apache.org> wrote:
> Author: ylavic
> Date: Thu Mar 23 21:34:25 2017
> New Revision: 1788334
>
> URL: http://svn.apache.org/viewvc?rev=1788334&view=rev
> Log:
> apr_allocator: Provide apr_allocator_align() to get the true size that
> would be allocated for the given size (including the header and alignment).
>
>
> Modified:
> apr/apr/trunk/CHANGES
> apr/apr/trunk/include/apr_allocator.h
> apr/apr/trunk/memory/unix/apr_pools.c
>
> Modified: apr/apr/trunk/CHANGES
> URL: http://svn.apache.org/viewvc/apr/apr/trunk/CHANGES?rev=1788334&r1=1788333&r2=1788334&view=diff
> ==============================================================================
> --- apr/apr/trunk/CHANGES [utf-8] (original)
> +++ apr/apr/trunk/CHANGES [utf-8] Thu Mar 23 21:34:25 2017
> @@ -1,6 +1,10 @@
> -*- coding: utf-8 -*-
> Changes for APR 2.0.0
>
> + *) apr_allocator: Provide apr_allocator_align() to get the true size that
> + would be allocated for the given size (including the header and
> + alignment). [Yann Ylavic]
> +
> *) apr_crypto: avoid excessive iteration in bcrypt hash.
> [Hanno Böck <hanno hboeck.de>]
>
>
> Modified: apr/apr/trunk/include/apr_allocator.h
> URL: http://svn.apache.org/viewvc/apr/apr/trunk/include/apr_allocator.h?rev=1788334&r1=1788333&r2=1788334&view=diff
> ==============================================================================
> --- apr/apr/trunk/include/apr_allocator.h (original)
> +++ apr/apr/trunk/include/apr_allocator.h Thu Mar 23 21:34:25 2017
> @@ -103,6 +103,14 @@ APR_DECLARE(apr_memnode_t *) apr_allocat
> APR_DECLARE(void) apr_allocator_free(apr_allocator_t *allocator,
> apr_memnode_t *memnode)
> __attribute__((nonnull(1,2)));
> +
> +/**
> + * Return the aligned (round up) size that an allocator would use for
> + * the given size.
> + * @param size The size to align
> + * @return The aligned size (or zero on apr_size_t overflow)
> + */
> +APR_DECLARE(apr_size_t) apr_allocator_align(apr_size_t size);
What is the purpose of this API? Currently caller may use
apr_memnode_t.endp to find actually allocated memory size.
Anyway I think it maybe worth to add apr_allocator_t argument to
apr_allocator_align() function even currently allocation alignment is
the same for all allocators.
--
Ivan Zhakov
Re: svn commit: r1788334 - in /apr/apr/trunk: CHANGES
include/apr_allocator.h memory/unix/apr_pools.c
Posted by Nick Kew <ni...@apache.org>.
On Mon, 2017-03-27 at 16:55 +0300, Ivan Zhakov wrote:
> > +
> > +/**
> > + * Return the aligned (round up) size that an allocator would use for
> > + * the given size.
> > + * @param size The size to align
> > + * @return The aligned size (or zero on apr_size_t overflow)
> > + */
> > +APR_DECLARE(apr_size_t) apr_allocator_align(apr_size_t size);
> What is the purpose of this API? Currently caller may use
> apr_memnode_t.endp to find actually allocated memory size.
>
> Anyway I think it maybe worth to add apr_allocator_t argument to
> apr_allocator_align() function even currently allocation alignment is
> the same for all allocators.
+1. If this is going anywhere as an API, it seems like a no-brainer.
--
Nick Kew
Re: svn commit: r1788334 - in /apr/apr/trunk: CHANGES
include/apr_allocator.h memory/unix/apr_pools.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 28 March 2017 at 20:24, Yann Ylavic <yl...@gmail.com> wrote:
> On Tue, Mar 28, 2017 at 2:39 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>>
>> I'm not sure that *actual* allocation size could be predicted in all
>> cases and possible apr_allocator_t implementation.
>
> Is there multiple apr_allocator_t implementations?
>
Currently at least two modes: heap-based and mmap-based.
>> Currently
>> apr_bucket_alloc_aligned_floor() and apr_bucket_alloc() logic should
>> be kept in sync, which is error prone imho.
>
> I'd say that thanks to apr_allocator_align() they at least do not
> depend on the allocator internals (look at the big assumption in the
> !APR_VERSION_AT_LEAST(1,6,0) case in
> apr_bucket_alloc_aligned_floor()...).
> If one changes SMALL_NODE_SIZE or SIZEOF_NODE_HEADER_T it should
> continue to work, but yes if the bucket_alloc implementation changes
> radically it may break (I hope that before such change one would grep
> SMALL_NODE_SIZE and SIZEOF_NODE_HEADER_T in the file :)
>
> I don't see how we couldn't implement any new version of
> apr_bucket_alloc_aligned_floor() though, some specific example of such
> a breaking change?
>
>>
>> After looking to apr_bucket_alloc code I suggest to add
>> apr_bucket_node_size(const void *node) that returns actual allocated
>> node size. apr_file_bucket implementation could allocate desired
>> buffer size and then query actual allocated size via
>> apr_bucket_node_size. Apache Serf bucket_allocator uses similar
>> approach.
>
> That's still after the allocation took place.
>
I think it's a good thing: it's always better to rely on actual
behavior, than try to predict it.
Do not forget that apr_allocator_t has at least two modes: use heap or use mmap.
> Let's say a user configures the buffer size of file buckets to 8192
> bytes (users like powers of two ;)
>
The opposite situation may happen if user configure size of buffer
like 12000 bytes: in this case actual buffer would be ~ 8100 bytes.
Also apr_allocator_t has list of free nodes and may return larger node
than requested. Why waste this memory?
> With the help apr_bucket_alloc_aligned_floor() we can allocate exactly
> 8192 bytes, yet have only ~8100 of them available for reads (a bit
> like APR_BUCKET_BUFF_SIZE=8000 takes allocators overhead into account,
> approximately...).
>
> With your method, we'd allocate 12288 bytes, but either use only 3/4
> of them (internal fragmentation), or really try to read ~12200 bytes,
> in both cases this is more than what the user asked.
> I tend to think that the configured value is an upper bound, more
> related to some controlled memory consumption (or some device
> capacity) than anything.
>
I'm not sure that user configures an upper bound.
--
Ivan Zhakov
Re: svn commit: r1788334 - in /apr/apr/trunk: CHANGES
include/apr_allocator.h memory/unix/apr_pools.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Tue, Mar 28, 2017 at 2:39 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>
> I'm not sure that *actual* allocation size could be predicted in all
> cases and possible apr_allocator_t implementation.
Is there multiple apr_allocator_t implementations?
> Currently
> apr_bucket_alloc_aligned_floor() and apr_bucket_alloc() logic should
> be kept in sync, which is error prone imho.
I'd say that thanks to apr_allocator_align() they at least do not
depend on the allocator internals (look at the big assumption in the
!APR_VERSION_AT_LEAST(1,6,0) case in
apr_bucket_alloc_aligned_floor()...).
If one changes SMALL_NODE_SIZE or SIZEOF_NODE_HEADER_T it should
continue to work, but yes if the bucket_alloc implementation changes
radically it may break (I hope that before such change one would grep
SMALL_NODE_SIZE and SIZEOF_NODE_HEADER_T in the file :)
I don't see how we couldn't implement any new version of
apr_bucket_alloc_aligned_floor() though, some specific example of such
a breaking change?
>
> After looking to apr_bucket_alloc code I suggest to add
> apr_bucket_node_size(const void *node) that returns actual allocated
> node size. apr_file_bucket implementation could allocate desired
> buffer size and then query actual allocated size via
> apr_bucket_node_size. Apache Serf bucket_allocator uses similar
> approach.
That's still after the allocation took place.
Let's say a user configures the buffer size of file buckets to 8192
bytes (users like powers of two ;)
With the help apr_bucket_alloc_aligned_floor() we can allocate exactly
8192 bytes, yet have only ~8100 of them available for reads (a bit
like APR_BUCKET_BUFF_SIZE=8000 takes allocators overhead into account,
approximately...).
With your method, we'd allocate 12288 bytes, but either use only 3/4
of them (internal fragmentation), or really try to read ~12200 bytes,
in both cases this is more than what the user asked.
I tend to think that the configured value is an upper bound, more
related to some controlled memory consumption (or some device
capacity) than anything.
Regards,
Yann.
Re: svn commit: r1788334 - in /apr/apr/trunk: CHANGES
include/apr_allocator.h memory/unix/apr_pools.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 28 March 2017 at 01:02, Yann Ylavic <yl...@gmail.com> wrote:
> On Mon, Mar 27, 2017 at 3:55 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>> On 24 March 2017 at 00:34, <yl...@apache.org> wrote:
>>> +
>>> +/**
>>> + * Return the aligned (round up) size that an allocator would use for
>>> + * the given size.
>>> + * @param size The size to align
>>> + * @return The aligned size (or zero on apr_size_t overflow)
>>> + */
>>> +APR_DECLARE(apr_size_t) apr_allocator_align(apr_size_t size);
>> What is the purpose of this API? Currently caller may use
>> apr_memnode_t.endp to find actually allocated memory size.
>
> The purpose is to get this size before allocating any memory.
> It is (newly) used in APR by apr_bucket_alloc_aligned_floor (itself
> used by apr_bucket_file_set_buf_size), where the purpose is to take
> into account and be precise with the overhead of the
> (bucket_)allocator(s), and avoid over-allocating (wasted) memory.
Hi Yann, thanks for explanation!
>
>>
>> Anyway I think it maybe worth to add apr_allocator_t argument to
>> apr_allocator_align() function even currently allocation alignment is
>> the same for all allocators.
>
> I see your point, but since there is no way to do this yet and that'd
> require another API change, maybe we could do this at that time?
> If you are concerned about the current name which would better fit an
> allocator argument, let's use apr_allocator_default_align() for now...
> or e.g. apr_allocator_align_of() later ;)
>
> Hmm, this makes me think that if apr_allocator_align() need an
> alloctor, apr_bucket_alloc_aligned_floor() needs a bucket_alloc too,
> and both functions may be less useful (or easy to use) if they require
> the allocator the memory will finally be allocated on (say
> apr_allocator_align() has to be used at the init time of an
> application where the runtime allocator is not created yet).
>
> Also why would one want a different allocator alignment since (s)he
> can apr_allocator_alloc() any size in the first place? The alignment
> looks more like a property of the system than the allocator itself for
> me...
I'm not sure that *actual* allocation size could be predicted in all
cases and possible apr_allocator_t implementation. Currently
apr_bucket_alloc_aligned_floor() and apr_bucket_alloc() logic should
be kept in sync, which is error prone imho.
After looking to apr_bucket_alloc code I suggest to add
apr_bucket_node_size(const void *node) that returns actual allocated
node size. apr_file_bucket implementation could allocate desired
buffer size and then query actual allocated size via
apr_bucket_node_size. Apache Serf bucket_allocator uses similar
approach.
--
Ivan Zhakov
Re: svn commit: r1788334 - in /apr/apr/trunk: CHANGES
include/apr_allocator.h memory/unix/apr_pools.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Mon, Mar 27, 2017 at 3:55 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On 24 March 2017 at 00:34, <yl...@apache.org> wrote:
>> +
>> +/**
>> + * Return the aligned (round up) size that an allocator would use for
>> + * the given size.
>> + * @param size The size to align
>> + * @return The aligned size (or zero on apr_size_t overflow)
>> + */
>> +APR_DECLARE(apr_size_t) apr_allocator_align(apr_size_t size);
> What is the purpose of this API? Currently caller may use
> apr_memnode_t.endp to find actually allocated memory size.
The purpose is to get this size before allocating any memory.
It is (newly) used in APR by apr_bucket_alloc_aligned_floor (itself
used by apr_bucket_file_set_buf_size), where the purpose is to take
into account and be precise with the overhead of the
(bucket_)allocator(s), and avoid over-allocating (wasted) memory.
>
> Anyway I think it maybe worth to add apr_allocator_t argument to
> apr_allocator_align() function even currently allocation alignment is
> the same for all allocators.
I see your point, but since there is no way to do this yet and that'd
require another API change, maybe we could do this at that time?
If you are concerned about the current name which would better fit an
allocator argument, let's use apr_allocator_default_align() for now...
or e.g. apr_allocator_align_of() later ;)
Hmm, this makes me think that if apr_allocator_align() need an
alloctor, apr_bucket_alloc_aligned_floor() needs a bucket_alloc too,
and both functions may be less useful (or easy to use) if they require
the allocator the memory will finally be allocated on (say
apr_allocator_align() has to be used at the init time of an
application where the runtime allocator is not created yet).
Also why would one want a different allocator alignment since (s)he
can apr_allocator_alloc() any size in the first place? The alignment
looks more like a property of the system than the allocator itself for
me...