Cool! Much better.
On Thu, Apr 22, 2010 at 19:11, <stsp_at_apache.org> wrote:
>...
> +++ subversion/trunk/subversion/libsvn_subr/subst.c Thu Apr 22 23:11:48 2010
>...
> + mt->saved_baton.in_baton = dup_translation_baton(b->in_baton, pool);
> + mt->saved_baton.out_baton = dup_translation_baton(b->out_baton, pool);
> + mt->saved_baton.written = b->written;
> + mt->saved_baton.readbuf = svn_stringbuf_dup(b->readbuf, pool);
> + mt->saved_baton.readbuf_off = b->readbuf_off;
> + mt->saved_baton.buf = apr_pmemdup(pool, b->buf, SVN__STREAM_CHUNK_SIZE + 1);
These allocations are all good, since it is the caller's pool being
used for these.
>...
> @@ -1242,23 +1216,21 @@ static svn_error_t *
> translated_stream_seek(void *baton, svn_stream_mark_t *mark)
> {
> struct translated_stream_baton *b = baton;
> - struct translated_stream_baton *sb;
> - mark_translated_t *mark_translated = (mark_translated_t *)mark;
> + mark_translated_t *mt = (mark_translated_t *)mark;
>
> /* Flush output buffer if necessary. */
> if (b->written)
> SVN_ERR(translate_chunk(b->stream, b->out_baton, NULL, 0, b->iterpool));
>
> - SVN_ERR(svn_stream_seek(b->stream, mark_translated->mark));
> + SVN_ERR(svn_stream_seek(b->stream, mt->mark));
>
> /* Restore translation state. */
> - sb = mark_translated->saved_baton;
> - b->in_baton = dup_translation_baton(sb->in_baton, b->pool);
> - b->out_baton = dup_translation_baton(sb->out_baton, b->pool);
> - b->written = sb->written;
> - b->readbuf = svn_stringbuf_dup(sb->readbuf, b->pool);
> - b->readbuf_off = sb->readbuf_off;
> - b->buf = apr_pmemdup(b->pool, sb->buf, SVN__STREAM_CHUNK_SIZE + 1);
> + b->in_baton = dup_translation_baton(mt->saved_baton.in_baton, b->pool);
> + b->out_baton = dup_translation_baton(mt->saved_baton.out_baton, b->pool);
> + b->written = mt->saved_baton.written;
> + b->readbuf = svn_stringbuf_dup(mt->saved_baton.readbuf, b->pool);
> + b->readbuf_off = mt->saved_baton.readbuf_off;
> + b->buf = apr_pmemdup(b->pool, mt->saved_baton.buf, SVN__STREAM_CHUNK_SIZE + 1);
But these three allocations concern me. Again, if a caller does a
repeated series of mark/seek, then the stream's pool continue to grow
without bound (and the caller has no control over this).
So the question is whether we can remove these additional allocations.
dup_translation_baton is just an alloc + struct copy. Since there are
no more sub-structures to deal with in a "dup", then I believe the
above two statements could simply be:
b->in_baton = mt->saved_baton.in_baton;
b->out_baton = mt->saved_baton.out_baton;
The readbuf allocation should be pretty simple since we know NUL
characters are not present in these streams. Thus, we could do the
following:
svn_stringbuf_set(b->readbuf, mt->saved_baton.readbuf);
If there will be NUL characters, then we could do other things.
We also know that b->buf and mt->saved_baton.buf are BOTH sized at
SVN__STREAM_CHUNK_SIZE + 1. Therefore, we could do a simple copy,
rather than create a new allocation for b->buf:
memcpy(b->buf, mt->saved_baton.buf, SVN__STREAM_CHUNK_SIZE + 1);
It might not be a bad idea to have a symbol for this. Within the
struct definition for translated_stream_baton, we could do something
like:
/* ... */
char *buf;
#define SVN__TRANSLATION_BUF_SIZE (SVN__STREAM_CHUNK_SIZE + 1)
And then include a mention in the 'buf' docstring noting its fixed
size, and to use that symbol.
With these changes, then a mark/seek should not increase the size of
the stream's pool. In fact, we can REMOVE the pool from the baton, so
that problems like this won't happen again. There is a reference in
the translation stream's close callback, but the user of the stream
can be left to deal with the lifetime (by clearing the pool passed to
svn_subst_translated_stream).
>...
Cheers,
-g
Received on 2010-04-23 02:22:18 CEST