[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r1333936 - in /subversion/trunk/subversion: include/private/svn_wc_private.h include/svn_ra.h libsvn_client/ra.c libsvn_ra_serf/update.c libsvn_wc/adm_ops.c

From: C. Michael Pilato <cmpilato_at_collab.net>
Date: Mon, 7 May 2012 17:22:22 -0400

On 05/04/2012 05:16 PM, Greg Stein wrote:
> On Fri, May 4, 2012 at 9:13 AM, <cmpilato_at_apache.org> wrote:
>> Author: cmpilato
>> Date: Fri May 4 13:13:00 2012
>> New Revision: 1333936

Okay. I think I've digested and acted open all of the feedback I've cropped
out of this response. There's just this one task remaining now:

> Note: we need to make the callback smarter. In your implementation, it
> opens the pristine contents and *holds it open* until the HEAD
> completes. If ra_serf queues 3000 HEAD requests... you now require
> 3000 file handles. Not a good idea.
> Also note: Mark already ran into the file handle problem.
> The solution is a custom stream.
> svn_wc__get_pristine_contents_by_checksum() should use
> svn_wc__db_pristine_check() to see if a pristine is present. If it
> *does*, then it returns a stream with a custom read function. On the
> first read, it will use svn_wc__db_pristine_read() to open the
> pristine content stream (and the underlying file handle). Then it just
> delegates reads to that inner stream.
> As a recommendation, I would suggest creating a "lazy open" stream
> which takes a callback that opens the stream upon first read. We are
> going to need this lazy-open stream during commits (the RA layer may
> choose not to deliver contents to the server, so we shouldn't bother
> opening them).

To be clear, you mean that we need something like:

typedef svn_error_t (*svn_stream_lazyopen_func_t)(svn_stream_t **stream,
                                                  void *baton,
                                                  apr_pool_t *result_pool,
                                                  apr_pool_t *scratch_pool);
svn_error_t *
svn_stream_lazyopen_create(svn_stream_t **stream,
                           svn_stream_lazyopen_func_t open_func,
                           void *open_baton,
                           apr_pool_t *result_pool,
                           apr_pool_t *scratch_pool);

In my specific situation, svn_wc__get_pristine_contents_by_checksum() would
call svn_stream_lazyopen_create(), passing an open_func callback which is
written to invoke svn_wc__db_pristine_read().

One downside of this is that svn_wc__db_pristine_read() promises that "even
if the pristine text is removed from the store while it is being read, the
stream will remain valid and readable until it is closed." That promise
becomes more valuable the earlier you open the stream to the pristine
contents; in this delayed open scenario, I anticipate that a "move"
operation from the server could see reduced benefit, even if the "add" side
of the move is processed before the "delete"). While parsing the REPORT,
the RA callback might say, "Yep, I've got those contents!" But by the time
the serf connection pipelining magic runs to actually read the contents,
prior delete handling might have blown them away.

Oh well, *if* that's even really what would happen, I suppose it still
doesn't outweight the current badness of having Mark running out of file
handles. :-P

C. Michael Pilato <cmpilato_at_collab.net>
CollabNet   <>   www.collab.net   <>   Enterprise Cloud Development
Received on 2012-05-07 23:22:59 CEST

This is an archived mail posted to the Subversion Dev mailing list.