You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by "Hyrum K. Wright" <hy...@mail.utexas.edu> on 2010/08/04 19:34:19 UTC
Re: svn commit: r982375 - in /subversion/branches/performance/subversion:
include/svn_io.h libsvn_subr/stream.c
On Wed, Aug 4, 2010 at 2:27 PM, <st...@apache.org> wrote:
> Author: stefan2
> Date: Wed Aug 4 19:27:41 2010
> New Revision: 982375
>
> URL: http://svn.apache.org/viewvc?rev=982375&view=rev
> Log:
> Introduce a variant of svn_stream_from_aprfile2, that takes cached file handles
> instead of APR file handles.
>
> * subversion/include/svn_io.h
> (svn_stream_from_aprfile3): declare new API function
Is this intended as a replacement for svn_stream_from_aprfile2()? If
so, the the later should be deprecated. If not, I would suggest a new
name, since we generally only have one non-deprecated version of
svn_fooN() APIs in any given release. It's okay for the separate
functions to exist, but they shouldn't be related via the naming
ancestry.
-Hyrum
> * subversion/libsvn_subr/stream.c
> (baton_apr): add cached_handle member
> (close_handler_cached_handle): new close() function
> (stream_from_aprfile): extract generic part from svn_stream_from_aprfile2
> (svn_stream_from_aprfile2): call stream_from_aprfile for most of the init code
> (svn_stream_from_aprfile3): implement new API function
>
> Modified:
> subversion/branches/performance/subversion/include/svn_io.h
> 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=982375&r1=982374&r2=982375&view=diff
> ==============================================================================
> --- subversion/branches/performance/subversion/include/svn_io.h (original)
> +++ subversion/branches/performance/subversion/include/svn_io.h Wed Aug 4 19:27:41 2010
> @@ -923,6 +923,26 @@ svn_stream_from_aprfile2(apr_file_t *fil
> svn_boolean_t disown,
> apr_pool_t *pool);
>
> +/* "forward-declare" svn_file_handle_cache__handle_t */
> +struct svn_file_handle_cache__handle_t;
> +
> +/** Create a stream from a cached file handle. For convenience, if @a file
> + * is @c NULL, an empty stream created by svn_stream_empty() is returned.
> + *
> + * This function should normally be called with @a disown set to FALSE,
> + * in which case closing the stream will also return the file handle to
> + * the respective cache object.
> + *
> + * If @a disown is TRUE, the stream will disown the file handle, meaning
> + * that svn_stream_close() will not close the cached file handle.
> + *
> + * @since New in 1.7.
> + */
> +svn_stream_t *
> +svn_stream_from_aprfile3(struct svn_file_handle_cache__handle_t *file,
> + svn_boolean_t disown,
> + apr_pool_t *pool);
> +
> /** Similar to svn_stream_from_aprfile2(), except that the file will
> * always be disowned.
> *
>
> Modified: subversion/branches/performance/subversion/libsvn_subr/stream.c
> URL: http://svn.apache.org/viewvc/subversion/branches/performance/subversion/libsvn_subr/stream.c?rev=982375&r1=982374&r2=982375&view=diff
> ==============================================================================
> --- subversion/branches/performance/subversion/libsvn_subr/stream.c (original)
> +++ subversion/branches/performance/subversion/libsvn_subr/stream.c Wed Aug 4 19:27:41 2010
> @@ -43,7 +43,7 @@
> #include "svn_checksum.h"
> #include "svn_path.h"
> #include "private/svn_eol_private.h"
> -
> +#include "private/svn_file_handle_cache.h"
>
> struct svn_stream_t {
> void *baton;
> @@ -579,6 +579,10 @@ struct baton_apr {
> apr_file_t *file;
> apr_pool_t *pool;
>
> + /* If not NULL, file is actually wrapped into this cached file handle
> + * and should be returned to the file handle cache asap. */
> + svn_file_handle_cache__handle_t *cached_handle;
> +
> /* Offsets when reading from a range of the file.
> * When either of these is negative, no range has been specified. */
> apr_off_t start;
> @@ -622,6 +626,17 @@ close_handler_apr(void *baton)
> return svn_io_file_close(btn->file, btn->pool);
> }
>
> +/* Returns the cached file handle back to the respective cache object.
> + * This is call is allowed even for btn->cached_handle == NULL.
> + */
> +static svn_error_t *
> +close_handler_cached_handle(void *baton)
> +{
> + struct baton_apr *btn = baton;
> +
> + return svn_file_handle_cache__close(btn->cached_handle);
> +}
> +
> static svn_error_t *
> reset_handler_apr(void *baton)
> {
> @@ -713,36 +728,83 @@ svn_stream_open_unique(svn_stream_t **st
> return SVN_NO_ERROR;
> }
>
> +/* Common initialization code for svn_stream_from_aprfile2() and
> + * svn_stream_from_aprfile3().
> + */
> +static svn_stream_t *
> +stream_from_aprfile(struct baton_apr **baton,
> + apr_file_t *file,
> + apr_pool_t *pool)
> +{
> + struct baton_apr *new_baton;
> + svn_stream_t *stream;
> +
> + /* create and fully initialize the baton */
> + new_baton = apr_palloc(pool, sizeof(*new_baton));
> + new_baton->file = file;
> + new_baton->cached_handle = NULL; /* default */
> + new_baton->pool = pool;
> + new_baton->start = -1;
> + new_baton->end = -1;
> +
> + /* construct the stream vtable, except for the close() function */
> + stream = svn_stream_create(new_baton, pool);
> + svn_stream_set_read(stream, read_handler_apr);
> + svn_stream_set_write(stream, write_handler_apr);
> + svn_stream_set_reset(stream, reset_handler_apr);
> + svn_stream_set_mark(stream, mark_handler_apr);
> + svn_stream_set_seek(stream, seek_handler_apr);
> +
> + /* return structures */
> + *baton = new_baton;
> +
> + return stream;
> +}
>
> svn_stream_t *
> svn_stream_from_aprfile2(apr_file_t *file,
> svn_boolean_t disown,
> apr_pool_t *pool)
> {
> + /* having no file at all is a special case */
> struct baton_apr *baton;
> - svn_stream_t *stream;
> -
> if (file == NULL)
> return svn_stream_empty(pool);
>
> - baton = apr_palloc(pool, sizeof(*baton));
> - baton->file = file;
> - baton->pool = pool;
> - baton->start = -1;
> - baton->end = -1;
> - stream = svn_stream_create(baton, pool);
> - svn_stream_set_read(stream, read_handler_apr);
> - svn_stream_set_write(stream, write_handler_apr);
> - svn_stream_set_reset(stream, reset_handler_apr);
> - svn_stream_set_mark(stream, mark_handler_apr);
> - svn_stream_set_seek(stream, seek_handler_apr);
> + /* construct and init the default stream structures */
> + svn_stream_t *stream = stream_from_aprfile(&baton, file, pool);
>
> + /* make sure to close the file handle after use if we own it */
> if (! disown)
> svn_stream_set_close(stream, close_handler_apr);
>
> return stream;
> }
>
> +svn_stream_t *
> +svn_stream_from_aprfile3(svn_file_handle_cache__handle_t *file,
> + svn_boolean_t disown,
> + apr_pool_t *pool)
> +{
> + struct baton_apr *baton;
> +
> + /* having no file at all is a special case (file == NULL is legal, too) */
> + apr_file_t *apr_file = svn_file_handle_cache__get_apr_handle(file);
> + if (apr_file == NULL)
> + return svn_stream_empty(pool);
> +
> + /* construct and init the default stream structures */
> + svn_stream_t *stream = stream_from_aprfile(&baton, apr_file, pool);
> +
> + /* store the cached file handle and return it to the cache after use,
> + * if we own it */
> + baton->cached_handle = file;
> + if (! disown)
> + svn_stream_set_close(stream, close_handler_cached_handle);
> +
> + return stream;
> +}
> +
> /* A read handler (#svn_read_fn_t) that forwards to read_handler_apr()
> but only allows reading between BATON->start and BATON->end. */
> static svn_error_t *
>
>
>
Re: svn commit: r982375 - in /subversion/branches/performance/subversion:
include/svn_io.h libsvn_subr/stream.c
Posted by Stefan Fuhrmann <st...@alice-dsl.de>.
Bert Huijben wrote:
> [Repeating the first mail then... Thanks tele2 for breaking my e-mail for
> the second time this week :(]
>
>
>
>> -----Original Message-----
>> From: stefan2@apache.org [mailto:stefan2@apache.org]
>> Sent: woensdag 4 augustus 2010 22:22
>> To: commits@subversion.apache.org
>> Subject: svn commit: r982391 - in
>> /subversion/branches/performance/subversion: include/svn_io.h
>> 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=982391&r1=982390&r2=982391&view=diff
>> =======================================================================
>> =======
>> --- subversion/branches/performance/subversion/include/svn_io.h
>> (original)
>> +++ subversion/branches/performance/subversion/include/svn_io.h Wed Aug
>> 4 20:22:10 2010
>> @@ -926,23 +926,6 @@ svn_stream_from_aprfile2(apr_file_t *fil
>> /* "forward-declare" svn_file_handle_cache__handle_t */
>> struct svn_file_handle_cache__handle_t;
>>
>
> If ^^^ this is a library private type (implied by the __).
>
> <snip>
>
>
>> +/** Create a stream from a cached file handle. For convenience, if @a
>> file
>> + * is @c NULL, an empty stream created by svn_stream_empty() is
>> returned.
>> + *
>> + * This function should normally be called with @a disown set to
>> FALSE,
>> + * in which case closing the stream will also return the file handle
>> to
>> + * the respective cache object.
>> + *
>> + * If @a disown is TRUE, the stream will disown the file handle,
>> meaning
>> + * that svn_stream_close() will not close the cached file handle.
>> + *
>> + * @since New in 1.7.
>> + */
>> +svn_stream_t *
>> +svn_stream_from_cached_file_handle
>> + (struct svn_file_handle_cache__handle_t *file,
>> + svn_boolean_t disown,
>> + apr_pool_t *pool);
>> +
>> /** Create a stream for reading from a range of an APR file.
>> * The stream cannot be written to.
>> *
>>
>
> Then this shouldn't be a public function, using that type.
>
> This function should then be moved to the cached file handle prefix and also
> use __ in its name.
>
>
> I don't think this API will have general use outside FSFS in 1.7? (But I can
> be mistaken)
>
Probably not.
> If not it should certainly be in a private header, to allow updating its api
> later without all our versioning rules for public APIs.
>
r983385 is the best I could come up with. It avoids code duplication
as well as a performance hit. However, it breaks the nice 1:1 relation
between source and header.
-- Stefan^2.
RE: svn commit: r982375 - in /subversion/branches/performance/subversion: include/svn_io.h libsvn_subr/stream.c
Posted by Bert Huijben <be...@qqmail.nl>.
[Repeating the first mail then... Thanks tele2 for breaking my e-mail for
the second time this week :(]
> -----Original Message-----
> From: stefan2@apache.org [mailto:stefan2@apache.org]
> Sent: woensdag 4 augustus 2010 22:22
> To: commits@subversion.apache.org
> Subject: svn commit: r982391 - in
> /subversion/branches/performance/subversion: include/svn_io.h
> 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=982391&r1=982390&r2=982391&view=diff
> =======================================================================
> =======
> --- subversion/branches/performance/subversion/include/svn_io.h
> (original)
> +++ subversion/branches/performance/subversion/include/svn_io.h Wed Aug
> 4 20:22:10 2010
> @@ -926,23 +926,6 @@ svn_stream_from_aprfile2(apr_file_t *fil
> /* "forward-declare" svn_file_handle_cache__handle_t */
> struct svn_file_handle_cache__handle_t;
If ^^^ this is a library private type (implied by the __).
<snip>
> +/** Create a stream from a cached file handle. For convenience, if @a
> file
> + * is @c NULL, an empty stream created by svn_stream_empty() is
> returned.
> + *
> + * This function should normally be called with @a disown set to
> FALSE,
> + * in which case closing the stream will also return the file handle
> to
> + * the respective cache object.
> + *
> + * If @a disown is TRUE, the stream will disown the file handle,
> meaning
> + * that svn_stream_close() will not close the cached file handle.
> + *
> + * @since New in 1.7.
> + */
> +svn_stream_t *
> +svn_stream_from_cached_file_handle
> + (struct svn_file_handle_cache__handle_t *file,
> + svn_boolean_t disown,
> + apr_pool_t *pool);
> +
> /** Create a stream for reading from a range of an APR file.
> * The stream cannot be written to.
> *
Then this shouldn't be a public function, using that type.
This function should then be moved to the cached file handle prefix and also
use __ in its name.
I don't think this API will have general use outside FSFS in 1.7? (But I can
be mistaken)
If not it should certainly be in a private header, to allow updating its api
later without all our versioning rules for public APIs.
Bert
RE: svn commit: r982375 - in /subversion/branches/performance/subversion: include/svn_io.h libsvn_subr/stream.c
Posted by Bert Huijben <be...@qqmail.nl>.
[Repeating the first mail then... Thanks tele2 for breaking my e-mail for
the second time this week :(]
> -----Original Message-----
> From: stefan2@apache.org [mailto:stefan2@apache.org]
> Sent: woensdag 4 augustus 2010 22:22
> To: commits@subversion.apache.org
> Subject: svn commit: r982391 - in
> /subversion/branches/performance/subversion: include/svn_io.h
> 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=982391&r1=982390&r2=982391&view=diff
> =======================================================================
> =======
> --- subversion/branches/performance/subversion/include/svn_io.h
> (original)
> +++ subversion/branches/performance/subversion/include/svn_io.h Wed Aug
> 4 20:22:10 2010
> @@ -926,23 +926,6 @@ svn_stream_from_aprfile2(apr_file_t *fil
> /* "forward-declare" svn_file_handle_cache__handle_t */
> struct svn_file_handle_cache__handle_t;
If ^^^ this is a library private type (implied by the __).
<snip>
> +/** Create a stream from a cached file handle. For convenience, if @a
> file
> + * is @c NULL, an empty stream created by svn_stream_empty() is
> returned.
> + *
> + * This function should normally be called with @a disown set to
> FALSE,
> + * in which case closing the stream will also return the file handle
> to
> + * the respective cache object.
> + *
> + * If @a disown is TRUE, the stream will disown the file handle,
> meaning
> + * that svn_stream_close() will not close the cached file handle.
> + *
> + * @since New in 1.7.
> + */
> +svn_stream_t *
> +svn_stream_from_cached_file_handle
> + (struct svn_file_handle_cache__handle_t *file,
> + svn_boolean_t disown,
> + apr_pool_t *pool);
> +
> /** Create a stream for reading from a range of an APR file.
> * The stream cannot be written to.
> *
Then this shouldn't be a public function, using that type.
This function should then be moved to the cached file handle prefix and also
use __ in its name.
I don't think this API will have general use outside FSFS in 1.7? (But I can
be mistaken)
If not it should certainly be in a private header, to allow updating its api
later without all our versioning rules for public APIs.
Bert
RE: svn commit: r982375 - in /subversion/branches/performance/subversion: include/svn_io.h libsvn_subr/stream.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: hyrum@hyrumwright.org [mailto:hyrum@hyrumwright.org] On Behalf Of
> Hyrum K. Wright
> Sent: woensdag 4 augustus 2010 21:34
> To: dev@subversion.apache.org
> Cc: commits@subversion.apache.org
> Subject: Re: svn commit: r982375 - in
> /subversion/branches/performance/subversion: include/svn_io.h
> libsvn_subr/stream.c
>
> On Wed, Aug 4, 2010 at 2:27 PM, <st...@apache.org> wrote:
> > Author: stefan2
> > Date: Wed Aug 4 19:27:41 2010
> > New Revision: 982375
> >
> > URL: http://svn.apache.org/viewvc?rev=982375&view=rev
> > Log:
> > Introduce a variant of svn_stream_from_aprfile2, that takes cached
> file handles
> > instead of APR file handles.
> >
> > * subversion/include/svn_io.h
> > (svn_stream_from_aprfile3): declare new API function
>
> Is this intended as a replacement for svn_stream_from_aprfile2()? If
> so, the the later should be deprecated. If not, I would suggest a new
> name, since we generally only have one non-deprecated version of
> svn_fooN() APIs in any given release. It's okay for the separate
> functions to exist, but they shouldn't be related via the naming
> ancestry.
(2nd followup).
If this is just a wrapper for open and close, you can just add your own
wrapper stream over the existing stream (forwarding to the real stream or
reimplementing the apr wrapper if it is performance critical to do that)
without changing the standard stream implementation. This is why we defined
the stream type in the first place.
In that case you can just move it into your own private header in
subversion/include/private/ with the rest of the file handle cache.
Bert
RE: svn commit: r982375 - in /subversion/branches/performance/subversion: include/svn_io.h libsvn_subr/stream.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: hyrum@hyrumwright.org [mailto:hyrum@hyrumwright.org] On Behalf Of
> Hyrum K. Wright
> Sent: woensdag 4 augustus 2010 21:34
> To: dev@subversion.apache.org
> Cc: commits@subversion.apache.org
> Subject: Re: svn commit: r982375 - in
> /subversion/branches/performance/subversion: include/svn_io.h
> libsvn_subr/stream.c
>
> On Wed, Aug 4, 2010 at 2:27 PM, <st...@apache.org> wrote:
> > Author: stefan2
> > Date: Wed Aug 4 19:27:41 2010
> > New Revision: 982375
> >
> > URL: http://svn.apache.org/viewvc?rev=982375&view=rev
> > Log:
> > Introduce a variant of svn_stream_from_aprfile2, that takes cached
> file handles
> > instead of APR file handles.
> >
> > * subversion/include/svn_io.h
> > (svn_stream_from_aprfile3): declare new API function
>
> Is this intended as a replacement for svn_stream_from_aprfile2()? If
> so, the the later should be deprecated. If not, I would suggest a new
> name, since we generally only have one non-deprecated version of
> svn_fooN() APIs in any given release. It's okay for the separate
> functions to exist, but they shouldn't be related via the naming
> ancestry.
(2nd followup).
If this is just a wrapper for open and close, you can just add your own
wrapper stream over the existing stream (forwarding to the real stream or
reimplementing the apr wrapper if it is performance critical to do that)
without changing the standard stream implementation. This is why we defined
the stream type in the first place.
In that case you can just move it into your own private header in
subversion/include/private/ with the rest of the file handle cache.
Bert