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

Re: [PATCH] Simplify and optimize RA-svn's string marshalling

From: Julian Foad <julian.foad_at_wandisco.com>
Date: Mon, 06 Jun 2011 13:23:49 +0100

I (Julian Foad) wrote:
> > On 03.06.2011 10:52, Julian Foad wrote:
> > > Hi Stefan2 and others.
> > >
> > > This patch simplifies code in RA-svn/marshal.c by using a single code
> > > path to read both short and long strings efficiently. Using a single
> > > code path is beneficial for test coverage.
> > >
> > > It is an alternative to r1028352 which merged r985606 and r1028092 from
> > > the performance branch.
> > >
> > > I have run the test suite over RA-svn (with BDB) but haven't done any
> > > more testing than that. It's just something I noticed while looking at
> > > that code, it's not solving a problem I've noticed or anything like
> > > that. By inspection I believe it is more efficient than
> > > read_long_string() was, and unchanged for short strings, but if you're
> > > able to do any performance analysis on it, that would be great.
>
> Daniel, thanks for your detailed review comments. I'll address them if
> we decide the patch is useful. But Stefan has another idea.
>
> Stefan Fuhrmann wrote:
> > Thanks for the patch. Currently, I 'm in the process
> > of moving and won't find time for a detailed review
> > and test until sometime next week.
> >
> > But maybe the right solution would be replacing
> > read_long_string() with a simple error return. That
> > is the only way to limit memory consumption on the
> > server side. If the protocol has been we implemented
> > correctly, no string should be longer than about 100k
> > (a svndiff window). So, rejecting everything above,
> > say, 16M should be fine.
>
> Sounds good. If we can set some maximum length above which the code
> should throw an error, then I offer the patch attached below instead. I
> need to rely on others to confirm that approach as I don't know that to
> be true.
>
> (The patch as attached here is made ignoring white space, so applying it
> will give wrong indentation. And I'd want to change the name of the max
> length constant as well.)

From IRC (edited):

julianf: Can anyone confirm there's a hard maximum length to an RA-svn
received string, something like delta window size plus a bit?
danielsh: I'm not aware of any hard limit in the protocol, but there may
be a 'practical' limit that no released client will exceed.
danielsh: See get-file in libsvn_ra_svn/protocol.
The protocol doc allows, theoretically, to send the entire file in one
string but I don't know if released clients do that.
julianf: In plain text, yes?
danielsh: yes.
julianf: In that case I say we can't apply a hard length limit on the
receiving end, because even if our software limits the sending end to
some max chunk size other svn-compatible software may not.
danielsh: get_file() in svnserve/serve.c is the implementation.
danielsh: IIRC, we don't promise on-the-wire compatibility for people
who reimplement the protocol
danielsh: we only promise compatibility for API users, and 'protocol' is
considered internal docs.
julianf: Maybe; but I thought/assumed the protocol doc was considered
official.
danielsh:
http://article.gmane.org/gmane.comp.version-control.subversion.devel/118801/match=protocol+compatible
julianf: Oh, OK.

In that linked email, David Glasser said in 2010 that "the Subversion
project's compatibility efforts don't apply to the svn wire protocols,
which are only documented for the convenience of developers. While we
aren't going to go out of our way to break third-party implementations
of the protocols, that can't necessarily be guaranteed."

- Julian
Received on 2011-06-06 14:24:26 CEST

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

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.