Re: svn commit: r936844 - in /subversion/trunk/subversion: libsvn_subr/subst.c tests/libsvn_subr/stream-test.c

[Greg Stein <gs...@gmail.com>]

On Thu, Apr 22, 2010 at 09:49,  <st...@apache.org> wrote:
>...
> +++ subversion/trunk/subversion/libsvn_subr/subst.c Thu Apr 22 13:49:14 2010
>...
> @@ -1164,13 +1201,40 @@ translated_stream_reset(void *baton)
>   return svn_error_return(err);
>  }
>
> +/* svn_stream_mark_t for translation streams. */
> +typedef struct
> +{
> +  /* Saved translation state. */
> +  struct translated_stream_baton *saved_baton;

You can lose the indirection here, and just embed one struct in
another. That'll save a separate allocation later, and various derefs.

> +  /* Mark set on the underlying stream. */
> +  svn_stream_mark_t *mark;
> +} mark_translated_t;
> +
>  /* Implements svn_io_mark_fn_t. */
>  static svn_error_t *
>  translated_stream_mark(void *baton, svn_stream_mark_t **mark, apr_pool_t *pool)
>  {
> +  mark_translated_t *mark_translated;
>   struct translated_stream_baton *b = baton;
> +  struct translated_stream_baton *sb;
> +
> +  mark_translated = apr_palloc(pool, sizeof(*mark_translated));
> +  SVN_ERR(svn_stream_mark(b->stream, &mark_translated->mark, pool));
> +
> +  /* Save translation state. */
> +  sb = apr_pcalloc(pool, sizeof(*sb));
> +  sb->in_baton = dup_translation_baton(b->in_baton, pool);
> +  sb->out_baton = dup_translation_baton(b->out_baton, pool);
> +  sb->written = b->written;
> +  sb->readbuf = svn_stringbuf_dup(b->readbuf, pool);
> +  sb->readbuf_off = b->readbuf_off;
> +  sb->buf = apr_pmemdup(pool, b->buf, SVN__STREAM_CHUNK_SIZE + 1);
>
> -  return svn_error_return(svn_stream_mark(b->stream, mark, pool));
> +  mark_translated->saved_baton = sb;

The above will all get easier with the embedded struct.

>...
> +  /* 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);

s/mark_translated/mt/, and the above becomes something like:

b->in_baton = dup_translation_baton(mt->save.in_baton, b->pool);

And with this said, I think you should stop copying the keywords
around. They are CONSTANT for the duration of the translation stream.
Thus, the individual marks can just refer to the stream's set of
keywords. That stream has a longer life than the marks, so we don't
need the keyword copying. (make sure to note this lifetime in some
comments somewhere!)

The problem with the copying is the following (in pseudocode):

  for i in range(1000000):
    mark = stream.mark()
    stream.reset_to(mark)

Each time you copy the keywords back into the stream, its pool just
gets larger...

This is why it is *TERRIBLY* important for objects not to carry around
pools. The translated_stream_baton should not be doing this!

>...
> +++ subversion/trunk/subversion/tests/libsvn_subr/stream-test.c Thu Apr 22 13:49:14 2010
>...
> +  len = 3;
> +  SVN_ERR(svn_stream_read(translated_stream, buf, &len));
> +  SVN_ERR_ASSERT(len == 3);
> +  buf[3] = '\0';
> +  SVN_ERR_ASSERT(strcmp(buf, "One") == 0);

You should be using SVN_TEST_ASSERT() and SVN_TEST_STRING_ASSERT() ...

>...

Cheers,
-g