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
Received on 2009-05-27 22:48:23 CEST