On 30.10.2010 15:09, Daniel Shahaf wrote:
[email protected] wrote on Sat, Oct 30, 2010 at 12:09:49 -0000:
Author: stefan2
Date: Sat Oct 30 12:09:49 2010
New Revision: 1029038
URL: http://svn.apache.org/viewvc?rev=1029038&view=rev
Log:
Improve on r1028104:
Move string buffer size alignment logic down the call stack.
Also, extend the logic to cover string growth as well.
* subversion/libsvn_subr/svn_string.c
(svn_stringbuf_ncreate): remove alignment logic
(svn_stringbuf_create_ensure): add it here
(svn_stringbuf_ensure): and here
Suggested by: Daniel Shahaf<d.s_at_daniel.shahaf.name>
You can say "Suggested by: danielsh" for people in COMMITTERS.
Modified:
subversion/branches/performance/subversion/libsvn_subr/svn_string.c
Modified: subversion/branches/performance/subversion/libsvn_subr/svn_string.c
URL:
http://svn.apache.org/viewvc/subversion/branches/performance/subversion/libsvn_subr/svn_string.c?rev=1029038&r1=1029037&r2=1029038&view=diff
==============================================================================
--- subversion/branches/performance/subversion/libsvn_subr/svn_string.c
(original)
+++ subversion/branches/performance/subversion/libsvn_subr/svn_string.c Sat Oct
30 12:09:49 2010
@@ -255,7 +255,13 @@ svn_stringbuf_create_empty(apr_pool_t *p
svn_stringbuf_t *
svn_stringbuf_create_ensure(apr_size_t blocksize, apr_pool_t *pool)
{
- char *data = apr_palloc(pool, ++blocksize); /* + space for '\0' */
+ /* apr_palloc will allocate multiples of 8.
+ * Thus, we would waste some of that memory if we stuck to the
+ * smaller size. Note that this is safe even if apr_palloc would
+ * use some other aligment or none at all. */
+
+ blocksize = APR_ALIGN_DEFAULT(++blocksize); /* + space for '\0' */
+ char *data = apr_palloc(pool, blocksize);
Two issues:
* Declaration ('char *data') after statement ('blocksize = ...').
We can't do this because we promise (and one buildbot enforces) C89
compatibility. Please avoid this.
Can't we just move to C++? ;)
* The blocksize line. It assigns to 'blocksize' and uses ++blocksize on
the same line... and on top of that, it uses ++blocksize as an
argument to a macro (and I don't know that that macro promises not to
multiply-evaluate its argument). All in all, it touches 'blocksize'
too many times for my poor brain. Could you change it to:
blocksize = APR_ALIGN_DEFAULT(blocksize+1);
please?
The ++part would have been ok for function calls because
the evaluation order is well-defined (assignment after r-value
calculation, increment before r-value calculation may access
blocksize).
However, you are right about the potential macro problem -
even if this macro will currently access the parameter only once.
Fixed in r1029090.
Assuming these two changes, +1 to merge to trunk. (Feel free to just
make the changes and merge them without waiting for another +1; if
further review is necessary, it can be done on trunk.)
Merged in r1029108.
-- Stefan^2.
