On Wed, May 27, 2009 at 4:48 PM, Hyrum K.
Wright<hyrum_wright_at_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
Received on 2009-06-10 16:55:58 CEST