You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by "Hyrum K. Wright" <hy...@hyrumwright.org> on 2009/05/27 20:48:00 UTC
Re: svn commit: r37868 - trunk/subversion/libsvn_subr
On May 27, 2009, at 3:17 PM, Paul T. Burba wrote:
> Author: pburba
> Date: Wed May 27 13:17:53 2009
> New Revision: 37868
>
> Log:
> Make svn_hash_read2 and svn_hash_read_incremental more memory
> efficient.
>
> * subversion/libsvn_subr/hash.c
> (hash_read): Use an iterpool for temporary allocations.
>
> Modified:
> trunk/subversion/libsvn_subr/hash.c
>
> Modified: trunk/subversion/libsvn_subr/hash.c
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_subr/hash.c?pathrev=37868&r1=37867&r2=37868
> =
> =
> =
> =
> =
> =
> =
> =
> ======================================================================
> --- trunk/subversion/libsvn_subr/hash.c Wed May 27 12:30:14 2009
> (r37867)
> +++ trunk/subversion/libsvn_subr/hash.c Wed May 27 13:17:53 2009
> (r37868)
> @@ -85,27 +85,37 @@ hash_read(apr_hash_t *hash, svn_stream_t
> svn_boolean_t eof;
> apr_size_t len, keylen, vallen;
> char c, *end, *keybuf, *valbuf;
> + svn_error_t *err = SVN_NO_ERROR;
> + apr_pool_t *iterpool = svn_pool_create(pool);
>
> while (1)
> {
> + svn_pool_clear(iterpool);
> +
> /* Read a key length line. Might be END, though. */
> - SVN_ERR(svn_stream_readline(stream, &buf, "\n", &eof, pool));
> + SVN_ERR(svn_stream_readline(stream, &buf, "\n", &eof,
> iterpool));
>
> /* Check for the end of the hash. */
> if ((!terminator && eof && buf->len == 0)
> || (terminator && (strcmp(buf->data, terminator) == 0)))
> - return SVN_NO_ERROR;
> + break;
>
> /* Check for unexpected end of stream */
> if (eof)
> - return svn_error_create(SVN_ERR_MALFORMED_FILE, NULL, NULL);
> + {
> + err = svn_error_create(SVN_ERR_MALFORMED_FILE, NULL, NULL);
> + break;
> + }
>
> if ((buf->len >= 3) && (buf->data[0] == 'K') && (buf->data[1]
> == ' '))
> {
> /* Get the length of the key */
> keylen = (size_t) strtoul(buf->data + 2, &end, 10);
> if (keylen == (size_t) ULONG_MAX || *end != '\0')
> - return svn_error_create(SVN_ERR_MALFORMED_FILE, NULL,
> NULL);
> + {
> + err = svn_error_create(SVN_ERR_MALFORMED_FILE, NULL,
> NULL);
> + break;
> + }
>
> /* Now read that much into a buffer. */
> keybuf = apr_palloc(pool, keylen + 1);
> @@ -116,18 +126,24 @@ hash_read(apr_hash_t *hash, svn_stream_t
> len = 1;
> SVN_ERR(svn_stream_read(stream, &c, &len));
> if (c != '\n')
> - return svn_error_create(SVN_ERR_MALFORMED_FILE, NULL,
> NULL);
> + {
> + err = svn_error_create(SVN_ERR_MALFORMED_FILE, NULL,
> NULL);
> + break;
> + }
>
> /* Read a val length line */
> - SVN_ERR(svn_stream_readline(stream, &buf, "\n", &eof,
> pool));
> + SVN_ERR(svn_stream_readline(stream, &buf, "\n", &eof,
> iterpool));
>
> if ((buf->data[0] == 'V') && (buf->data[1] == ' '))
> {
> vallen = (size_t) strtoul(buf->data + 2, &end, 10);
> if (vallen == (size_t) ULONG_MAX || *end != '\0')
> - return svn_error_create(SVN_ERR_MALFORMED_FILE,
> NULL, NULL);
> + {
> + err = svn_error_create(SVN_ERR_MALFORMED_FILE,
> NULL, NULL);
> + break;
> + }
>
> - valbuf = apr_palloc(pool, vallen + 1);
> + valbuf = apr_palloc(iterpool, vallen + 1);
> SVN_ERR(svn_stream_read(stream, valbuf, &vallen));
> valbuf[vallen] = '\0';
>
> @@ -135,14 +151,20 @@ hash_read(apr_hash_t *hash, svn_stream_t
> len = 1;
> SVN_ERR(svn_stream_read(stream, &c, &len));
> if (c != '\n')
> - return svn_error_create(SVN_ERR_MALFORMED_FILE,
> NULL, NULL);
> + {
> + err = svn_error_create(SVN_ERR_MALFORMED_FILE,
> NULL, NULL);
> + break;
> + }
>
> /* Add a new hash entry. */
> apr_hash_set(hash, keybuf, keylen,
> svn_string_ncreate(valbuf, vallen, pool));
> }
> else
> - return svn_error_create(SVN_ERR_MALFORMED_FILE, NULL,
> NULL);
> + {
> + err = svn_error_create(SVN_ERR_MALFORMED_FILE, NULL,
> NULL);
> + break;
> + }
> }
> else if (incremental && (buf->len >= 3)
> && (buf->data[0] == 'D') && (buf->data[1] == ' '))
> @@ -153,7 +175,7 @@ hash_read(apr_hash_t *hash, svn_stream_t
> return svn_error_create(SVN_ERR_MALFORMED_FILE, NULL,
> NULL);
>
> /* Now read that much into a buffer. */
> - keybuf = apr_palloc(pool, keylen + 1);
> + keybuf = apr_palloc(iterpool, keylen + 1);
> SVN_ERR(svn_stream_read(stream, keybuf, &keylen));
> keybuf[keylen] = '\0';
>
> @@ -161,14 +183,23 @@ hash_read(apr_hash_t *hash, svn_stream_t
> len = 1;
> SVN_ERR(svn_stream_read(stream, &c, &len));
> if (c != '\n')
> - return svn_error_create(SVN_ERR_MALFORMED_FILE, NULL,
> NULL);
> + {
> + err = svn_error_create(SVN_ERR_MALFORMED_FILE, NULL,
> NULL);
> + break;
> + }
>
> /* Remove this hash entry. */
> apr_hash_set(hash, keybuf, keylen, NULL);
> }
> else
> - return svn_error_create(SVN_ERR_MALFORMED_FILE, NULL, NULL);
> + {
> + err = svn_error_create(SVN_ERR_MALFORMED_FILE, NULL, NULL);
> + break;
> + }
> }
> +
> + svn_pool_destroy(iterpool);
> + return err;
Not a big deal, but the above should be:
return svn_error_return(err)
This ensures we get proper debug tracing even when directly returning
errors.
> ...
I'm a little unsure why the whole break-on-error dance was needed. We
have an pretty established habit of just returning errors from within
loops and not destroying the loop pool. Usually, the loop pool isn't
very big, and it'll get destroyed along with it's parent when the
error gets handled.
-Hyrum
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2355931
Re: svn commit: r37868 - trunk/subversion/libsvn_subr
Posted by Greg Stein <gs...@gmail.com>.
On Wed, May 27, 2009 at 22:48, Hyrum K. Wright<hy...@hyrumwright.org> wrote:
> On May 27, 2009, at 3:17 PM, Paul T. Burba wrote:
>...
>> + svn_pool_destroy(iterpool);
>> + return err;
>
> Not a big deal, but the above should be:
> return svn_error_return(err)
>
> This ensures we get proper debug tracing even when directly returning
> errors.
In this case, just saying "return err;" is fine. Consider that "err"
is created within this function, with "good" file/line values. There
is no additional tracing needed.
(and yes, I know it was later removed; point is that "return
svn_error_return(err);" is not *always* proper)
>...
And if this function is trying to be smarter about efficiency, pool
usage, etc, then it should avoid svn_string_ncreate() and its
alloc/copy. It can alloc the buffer in the result pool, and manually
alloc/assemble an svn_string_t in the result pool using that buffer.
Cheers,
-g
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2368804
Re: svn commit: r37868 - trunk/subversion/libsvn_subr
Posted by Paul Burba <pt...@gmail.com>.
On Wed, May 27, 2009 at 4:48 PM, Hyrum K.
Wright<hy...@mail.utexas.edu> wrote:
>
> On May 27, 2009, at 3:17 PM, Paul T. Burba wrote:
>
>> Author: pburba
>> Date: Wed May 27 13:17:53 2009
>> New Revision: 37868
>>
>> Log:
>> Make svn_hash_read2 and svn_hash_read_incremental more memory efficient.
>>
>> * subversion/libsvn_subr/hash.c
>> (hash_read): Use an iterpool for temporary allocations.
>>
>> Modified:
>> trunk/subversion/libsvn_subr/hash.c
>>
>> Modified: trunk/subversion/libsvn_subr/hash.c
>> URL:
>> http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_subr/hash.c?pathrev=37868&r1=37867&r2=37868
>>
>> ==============================================================================
>> --- trunk/subversion/libsvn_subr/hash.c Wed May 27 12:30:14 2009
>> (r37867)
>> +++ trunk/subversion/libsvn_subr/hash.c Wed May 27 13:17:53 2009
>> (r37868)
>> @@ -85,27 +85,37 @@ hash_read(apr_hash_t *hash, svn_stream_t
>> svn_boolean_t eof;
>> apr_size_t len, keylen, vallen;
>> char c, *end, *keybuf, *valbuf;
>> + svn_error_t *err = SVN_NO_ERROR;
>> + apr_pool_t *iterpool = svn_pool_create(pool);
>>
>> while (1)
>> {
>> + svn_pool_clear(iterpool);
>> +
>> /* Read a key length line. Might be END, though. */
>> - SVN_ERR(svn_stream_readline(stream, &buf, "\n", &eof, pool));
>> + SVN_ERR(svn_stream_readline(stream, &buf, "\n", &eof, iterpool));
>>
>> /* Check for the end of the hash. */
>> if ((!terminator && eof && buf->len == 0)
>> || (terminator && (strcmp(buf->data, terminator) == 0)))
>> - return SVN_NO_ERROR;
>> + break;
>>
>> /* Check for unexpected end of stream */
>> if (eof)
>> - return svn_error_create(SVN_ERR_MALFORMED_FILE, NULL, NULL);
>> + {
>> + err = svn_error_create(SVN_ERR_MALFORMED_FILE, NULL, NULL);
>> + break;
>> + }
>>
>> if ((buf->len >= 3) && (buf->data[0] == 'K') && (buf->data[1] == '
>> '))
>> {
>> /* Get the length of the key */
>> keylen = (size_t) strtoul(buf->data + 2, &end, 10);
>> if (keylen == (size_t) ULONG_MAX || *end != '\0')
>> - return svn_error_create(SVN_ERR_MALFORMED_FILE, NULL, NULL);
>> + {
>> + err = svn_error_create(SVN_ERR_MALFORMED_FILE, NULL, NULL);
>> + break;
>> + }
>>
>> /* Now read that much into a buffer. */
>> keybuf = apr_palloc(pool, keylen + 1);
>> @@ -116,18 +126,24 @@ hash_read(apr_hash_t *hash, svn_stream_t
>> len = 1;
>> SVN_ERR(svn_stream_read(stream, &c, &len));
>> if (c != '\n')
>> - return svn_error_create(SVN_ERR_MALFORMED_FILE, NULL, NULL);
>> + {
>> + err = svn_error_create(SVN_ERR_MALFORMED_FILE, NULL, NULL);
>> + break;
>> + }
>>
>> /* Read a val length line */
>> - SVN_ERR(svn_stream_readline(stream, &buf, "\n", &eof, pool));
>> + SVN_ERR(svn_stream_readline(stream, &buf, "\n", &eof,
>> iterpool));
>>
>> if ((buf->data[0] == 'V') && (buf->data[1] == ' '))
>> {
>> vallen = (size_t) strtoul(buf->data + 2, &end, 10);
>> if (vallen == (size_t) ULONG_MAX || *end != '\0')
>> - return svn_error_create(SVN_ERR_MALFORMED_FILE, NULL,
>> NULL);
>> + {
>> + err = svn_error_create(SVN_ERR_MALFORMED_FILE, NULL,
>> NULL);
>> + break;
>> + }
>>
>> - valbuf = apr_palloc(pool, vallen + 1);
>> + valbuf = apr_palloc(iterpool, vallen + 1);
>> SVN_ERR(svn_stream_read(stream, valbuf, &vallen));
>> valbuf[vallen] = '\0';
>>
>> @@ -135,14 +151,20 @@ hash_read(apr_hash_t *hash, svn_stream_t
>> len = 1;
>> SVN_ERR(svn_stream_read(stream, &c, &len));
>> if (c != '\n')
>> - return svn_error_create(SVN_ERR_MALFORMED_FILE, NULL,
>> NULL);
>> + {
>> + err = svn_error_create(SVN_ERR_MALFORMED_FILE, NULL,
>> NULL);
>> + break;
>> + }
>>
>> /* Add a new hash entry. */
>> apr_hash_set(hash, keybuf, keylen,
>> svn_string_ncreate(valbuf, vallen, pool));
>> }
>> else
>> - return svn_error_create(SVN_ERR_MALFORMED_FILE, NULL, NULL);
>> + {
>> + err = svn_error_create(SVN_ERR_MALFORMED_FILE, NULL, NULL);
>> + break;
>> + }
>> }
>> else if (incremental && (buf->len >= 3)
>> && (buf->data[0] == 'D') && (buf->data[1] == ' '))
>> @@ -153,7 +175,7 @@ hash_read(apr_hash_t *hash, svn_stream_t
>> return svn_error_create(SVN_ERR_MALFORMED_FILE, NULL, NULL);
>>
>> /* Now read that much into a buffer. */
>> - keybuf = apr_palloc(pool, keylen + 1);
>> + keybuf = apr_palloc(iterpool, keylen + 1);
>> SVN_ERR(svn_stream_read(stream, keybuf, &keylen));
>> keybuf[keylen] = '\0';
>>
>> @@ -161,14 +183,23 @@ hash_read(apr_hash_t *hash, svn_stream_t
>> len = 1;
>> SVN_ERR(svn_stream_read(stream, &c, &len));
>> if (c != '\n')
>> - return svn_error_create(SVN_ERR_MALFORMED_FILE, NULL, NULL);
>> + {
>> + err = svn_error_create(SVN_ERR_MALFORMED_FILE, NULL, NULL);
>> + break;
>> + }
>>
>> /* Remove this hash entry. */
>> apr_hash_set(hash, keybuf, keylen, NULL);
>> }
>> else
>> - return svn_error_create(SVN_ERR_MALFORMED_FILE, NULL, NULL);
>> + {
>> + err = svn_error_create(SVN_ERR_MALFORMED_FILE, NULL, NULL);
>> + break;
>> + }
>> }
>> +
>> + svn_pool_destroy(iterpool);
>> + return err;
>
> Not a big deal, but the above should be:
> return svn_error_return(err)
>
> This ensures we get proper debug tracing even when directly returning
> errors.
>
>> ...
>
> I'm a little unsure why the whole break-on-error dance was needed. We have
> an pretty established habit of just returning errors from within loops and
> not destroying the loop pool. Usually, the loop pool isn't very big, and
> it'll get destroyed along with it's parent when the error gets handled.
Hi Hyrum,
Agreed, I was getting a bit out of hand with pool cleanup there.
r37979 returns to previous behavior of returning error from within the
loop.
Thanks,
Paul
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2360947