You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by st...@apache.org on 2010/08/14 15:20:22 UTC
svn commit: r985487 - in /subversion/branches/performance/subversion:
include/svn_io.h libsvn_subr/io.c libsvn_subr/stream.c
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);
+
+
/** 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 *
Re: svn commit: r985487 - in
/subversion/branches/performance/subversion: include/svn_io.h
libsvn_subr/io.c libsvn_subr/stream.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Stefan Fuhrmann wrote on Tue, Aug 17, 2010 at 23:39:37 +0200:
> Daniel Shahaf wrote:
>> stefan2@apache.org wrote on Sat, Aug 14, 2010 at 13:20:22 -0000:
>>> URL: http://svn.apache.org/viewvc?rev=985487&view=rev
>>> +++ subversion/branches/performance/subversion/include/svn_io.h Sat Aug 14 13:20:22 2010
>>> +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?
>>
> By the time I wrote that code almost 3 months ago, I tried to minimize
> the impact (code churn) on the code base.
When an API is revved, marking its old version deprecated and rewriting
it as a wrapper isn't considered churn.
Upgrading all existing calls is a bit more noise, but in general we
still do it (although not in tests). Usually I try to do that in
a separate 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
>>
> Took me a number of commits but it should be o.k. by r986492.
>> Why you didn't make svn_io_file_read_full() a deprecated wrapper around
>> svn_io_file_read_full2()?
>>
> See above ;)
>
Thanks! And I hope you don't see this as procedural nitpicks --- I was
just highlighting the conventions we follow when revving an API.
> -- Stefan^2.
In other news: I'm a bit concerned about not seeing much
activity/discussions about merging some of your work to trunk. Perhaps
it'll be a good idea to start reviewing and merging some parts of it
now? (in parallel to your committing further new work to the branch)
For a change to be applied to trunk, you'll have to get a full
committer's +1 on that change.
Re: svn commit: r985487 - in /subversion/branches/performance/subversion:
include/svn_io.h libsvn_subr/io.c libsvn_subr/stream.c
Posted by Stefan Fuhrmann <st...@alice-dsl.de>.
Daniel Shahaf wrote:
> stefan2@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?
>
By the time I wrote that code almost 3 months ago, I tried to minimize
the impact (code churn) on the code base.
> 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
>
Took me a number of commits but it should be o.k. by r986492.
> Why you didn't make svn_io_file_read_full() a deprecated wrapper around
> svn_io_file_read_full2()?
>
See above ;)
-- Stefan^2.
Re: svn commit: r985487 - in
/subversion/branches/performance/subversion: include/svn_io.h
libsvn_subr/io.c libsvn_subr/stream.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
stefan2@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 *
>
>