stefan2_at_apache.org wrote on Sat, Aug 14, 2010 at 13:20:22 -0000:
> Author: stefan2
> Date: Sat Aug 14 13:20:22 2010
> New Revision: 985487
>
> URL: http://svn.apache.org/viewvc?rev=985487&view=rev
> Log:
> APR file I/O is a high frequency operation as the respective streams directly map their requests
> onto the file layer. Thus, introduce svn_io_file_putc() as symmetrical counterpart to *_getc()
> and use both for single char read requests because their APR implementation tends to involve
> far less overhead.
>
> Also, introduce and use svn_io_file_read_full2 that optionally doesn't create an error object
> upon EOF that may be discarded by the caller anyways. This actually translates into significant
> runtime savings because constructing the svn_error_t for file errors involves expensive locale
> dependent functions.
>
> * subversion/include/svn_io.h
> (svn_io_file_putc, svn_io_file_read_full2): declare new API functions
> * subversion/libsvn_subr/svn_io.c
> (svn_io_file_putc, svn_io_file_read_full2): implement new API functions
> * subversion/libsvn_subr/stream.c
> (read_handler_apr, write_handler_apr): use the I/O function best suitable for the request
>
> Modified:
> subversion/branches/performance/subversion/include/svn_io.h
> subversion/branches/performance/subversion/libsvn_subr/io.c
> subversion/branches/performance/subversion/libsvn_subr/stream.c
>
> Modified: subversion/branches/performance/subversion/include/svn_io.h
> URL: http://svn.apache.org/viewvc/subversion/branches/performance/subversion/include/svn_io.h?rev=985487&r1=985486&r2=985487&view=diff
> ==============================================================================
> --- subversion/branches/performance/subversion/include/svn_io.h (original)
> +++ subversion/branches/performance/subversion/include/svn_io.h Sat Aug 14 13:20:22 2010
> @@ -1745,6 +1745,15 @@ svn_io_file_getc(char *ch,
> apr_pool_t *pool);
>
>
> +/** Wrapper for apr_file_putc().
> + * @since New in 1.7
> + */
> +svn_error_t *
> +svn_io_file_putc(char ch,
> + apr_file_t *file,
> + apr_pool_t *pool);
> +
> +
> /** Wrapper for apr_file_info_get(). */
> svn_error_t *
> svn_io_file_info_get(apr_finfo_t *finfo,
> @@ -1770,6 +1779,20 @@ svn_io_file_read_full(apr_file_t *file,
> apr_pool_t *pool);
>
>
> +/** Wrapper for apr_file_read_full().
> + * If eof_is_ok is set, no svn_error_t error object
> + * will be created upon EOF.
> + * @since New in 1.7
> + */
> +svn_error_t *
> +svn_io_file_read_full2(apr_file_t *file,
> + void *buf,
> + apr_size_t nbytes,
> + apr_size_t *bytes_read,
> + svn_boolean_t eof_is_ok,
> + apr_pool_t *pool);
> +
> +
Why didn't you deprecate svn_io_file_read_full() in the same commit?
Usually we do it as follows:
+ the declaration of svn_io_file_read_full2() is *above* that
of svn_io_file_read_full (not critical)
+ mark the old function SVN_DEPRECATED (preprocessor symbol) and
doxygen @deprecated
+ change the doc string of the old function to be a diff against the new
function's docs
+ in the appropriate *.c file, upgrade the definition in-place
+ re-define the old function in lib_$foo/deprecated.c as a wrapper around the
new function
Why you didn't make svn_io_file_read_full() a deprecated wrapper around
svn_io_file_read_full2()?
Daniel
> /** Wrapper for apr_file_seek(). */
> svn_error_t *
> svn_io_file_seek(apr_file_t *file,
>
> Modified: subversion/branches/performance/subversion/libsvn_subr/io.c
> URL: http://svn.apache.org/viewvc/subversion/branches/performance/subversion/libsvn_subr/io.c?rev=985487&r1=985486&r2=985487&view=diff
> ==============================================================================
> --- subversion/branches/performance/subversion/libsvn_subr/io.c (original)
> +++ subversion/branches/performance/subversion/libsvn_subr/io.c Sat Aug 14 13:20:22 2010
> @@ -2856,6 +2856,17 @@ svn_io_file_getc(char *ch, apr_file_t *f
>
>
> svn_error_t *
> +svn_io_file_putc(char ch, apr_file_t *file, apr_pool_t *pool)
> +{
> + return do_io_file_wrapper_cleanup
> + (file, apr_file_putc(ch, file),
> + N_("Can't write file '%s'"),
> + N_("Can't write stream"),
> + pool);
> +}
> +
> +
> +svn_error_t *
> svn_io_file_info_get(apr_finfo_t *finfo, apr_int32_t wanted,
> apr_file_t *file, apr_pool_t *pool)
> {
> @@ -2893,6 +2904,24 @@ svn_io_file_read_full(apr_file_t *file,
>
>
> svn_error_t *
> +svn_io_file_read_full2(apr_file_t *file, void *buf,
> + apr_size_t nbytes, apr_size_t *bytes_read,
> + svn_boolean_t eof_is_ok,
> + apr_pool_t *pool)
> +{
> + apr_status_t status = apr_file_read_full(file, buf, nbytes, bytes_read);
> + if (APR_STATUS_IS_EOF(status) && eof_is_ok)
> + return SVN_NO_ERROR;
> +
> + return do_io_file_wrapper_cleanup
> + (file, status,
> + N_("Can't read file '%s'"),
> + N_("Can't read stream"),
> + pool);
> +}
> +
> +
> +svn_error_t *
> svn_io_file_seek(apr_file_t *file, apr_seek_where_t where,
> apr_off_t *offset, apr_pool_t *pool)
> {
>
> Modified: subversion/branches/performance/subversion/libsvn_subr/stream.c
> URL: http://svn.apache.org/viewvc/subversion/branches/performance/subversion/libsvn_subr/stream.c?rev=985487&r1=985486&r2=985487&view=diff
> ==============================================================================
> --- subversion/branches/performance/subversion/libsvn_subr/stream.c (original)
> +++ subversion/branches/performance/subversion/libsvn_subr/stream.c Sat Aug 14 13:20:22 2010
> @@ -600,12 +600,15 @@ read_handler_apr(void *baton, char *buff
> struct baton_apr *btn = baton;
> svn_error_t *err;
>
> - err = svn_io_file_read_full(btn->file, buffer, *len, len, btn->pool);
> - if (err && APR_STATUS_IS_EOF(err->apr_err))
> + if (*len == 1)
> {
> - svn_error_clear(err);
> - err = SVN_NO_ERROR;
> - }
> + err = svn_io_file_getc (buffer, btn->file, btn->pool);
> + if (err)
> + *len = 0;
> + }
> + else
> + err = svn_io_file_read_full2(btn->file, buffer, *len, len,
> + TRUE, btn->pool);
>
> return err;
> }
> @@ -614,8 +617,18 @@ static svn_error_t *
> write_handler_apr(void *baton, const char *data, apr_size_t *len)
> {
> struct baton_apr *btn = baton;
> + svn_error_t *err;
> +
> + if (*len == 1)
> + {
> + err = svn_io_file_putc (*data, btn->file, btn->pool);
> + if (err)
> + *len = 0;
> + }
> + else
> + err = svn_io_file_write_full(btn->file, data, *len, len, btn->pool);
>
> - return svn_io_file_write_full(btn->file, data, *len, len, btn->pool);
> + return err;
> }
>
> static svn_error_t *
>
>
Received on 2010-08-15 22:28:46 CEST