The current code works in both scenarios (tested via buffersizes of just a few bytes, and a debugger verifyimg the corner cases, etc). But I doubt it is worth all the trouble. The file is null until something is spilled and from that point onwards everything is in the file (until the spill is empty).
If you want to look at improving it now, feel free. I'm first working on switching the update logic to the new xml parser. (And will perform a further cleanup including these points round after that).
Thanks for your reviews!
Bert
Sent from Windows Mail
From: Ivan Zhakov
Sent: Monday, January 6, 2014 11:26 PM
To: Subversion Development, Bert Huijben
On 6 January 2014 15:23, <rhuijben_at_apache.org> wrote:
> Author: rhuijben
> Date: Mon Jan 6 11:23:05 2014
> New Revision: 1555716
>
> URL: http://svn.apache.org/r1555716
> Log:
> In the ra serf (update) editor, spool the report to a spillbuffer instead of
> to a tempfile to avoid creating a tempfile for small requests.
>
> + if (body_file != NULL)
> + {
> + /* The spillbuffer was spooled to disk. Use the most optimized way
> + * to send it to serf, like when we didn't spool to memory first */
> + apr_off_t offset;
> +
> + /* We need to flush the file, make it unbuffered (so that it can be
> + * zero-copied via mmap), and reset the position before attempting to
> + * deliver the file.
> + *
> + * N.B. If we have APR 1.3+, we can unbuffer the file to let us use mmap
> + * and zero-copy the PUT body. However, on older APR versions, we can't
> + * check the buffer status; but serf will fall through and create a file
> + * bucket for us on the buffered svndiff handle.
> + *
> + * ### Is this really a useful optimization for an update report?
> + */
> + SVN_ERR(svn_io_file_flush(body_file, pool));
> +#if APR_VERSION_AT_LEAST(1, 3, 0)
> + apr_file_buffer_set(body_file, NULL, 0);
> +#endif
>
> - *body_bkt = serf_bucket_file_create(report->body_file, alloc);
> + offset = 0;
> + SVN_ERR(svn_io_file_seek(body_file, APR_SET, &offset, pool));
> +
> + *body_bkt = serf_bucket_file_create(body_file, alloc);
> + }
> + else
> + {
> + /* Everything is already in memory. Just wrap as bucket.
> + * Note that this would just work for the file case if needed */
> + *body_bkt = svn_ra_serf__create_sb_bucket(report->body_sb, alloc,
> + report->pool, pool);
> + }
Hi Bert.
Probably this logic should be moved to svn_ra_serf__create_sb_bucket()?
Also I think that current implementation shouldn't work because
spillbuf file doesn't include in-memory content by default, because
SPILL_ALL_CONTENTS is FALSE. Did you test this scenario?
The proper implementation should create aggregate bucket with memory
and file buckets. But I'm not sure that all this optimization worth.
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Received on 2014-01-07 09:50:03 CET