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

Re: svn commit: r1698359 - in /subversion/trunk/subversion: include/svn_io.h libsvn_subr/stream.c svnadmin/svnadmin.c svnfsfs/load-index-cmd.c tests/libsvn_subr/stream-test.c

From: Stefan Fuhrmann <stefan.fuhrmann_at_wandisco.com>
Date: Wed, 2 Sep 2015 12:50:14 +0100

On Tue, Sep 1, 2015 at 7:26 PM, Evgeny Kotkov <evgeny.kotkov_at_visualsvn.com>

> Stefan Fuhrmann <stefan.fuhrmann_at_wandisco.com> writes:
> > Yes. This is exactly why we can only use it when we have reasonable
> control
> > over the stream's usage, i.e. we can use it in our CL tools because all
> the
> > code that will be run is under our control. But we cannot make e.g.
> > svn_stream_for_stdin() use it by default.
> [...]
> > The best solution seems to be to allow for explicit resource management
> as
> > we do with other potentially "expensive" objects. r1700305 implements
> that.
> I have several concerns about these changes (r1698359 and r1700305):
> 1) The new public API — svn_stream_wrap_buffered_read() — is dangerous to
> use, and wrong usage causes unbounded memory consumption in seemingly
> harmless situations. [...]

This is also true for svn_stringbuf_t, property lists and changed paths

2) The new API is backward incompatible.
> The problem is not only about how we handle this in our codebase.
> Existing
> API users have no idea about the fact that having svn_stream_mark_t
> objects
> could be "expensive", and about the existence of
> svn_stream_remove_mark().


Some of them already *are* expensive. See translated_stream_mark()
and to some degree stream_mark() in jni_io_stream.cpp.

> 3) The behavior of pool-based reference counting is unpredictable.

It is no more unpredictable than open file handles, right?
You have to close or clean up all of them before you can
e.g. delete the file in Windows.

> Given the above, I am -1 on this optimization for svnadmin load-revprops
> that
> was implemented in r1698359 and r1700305. Please revert these changes.

Thinking about alternative solutions I found that simply
having a buffering wrapper without mark support would
still eliminate the OS overhead and parts of the internal
overhead. That would address all the points you have
made above while still providing a decent improvement.

IOW, revert r1700305 and rework / reduce / simplify the
code changed by r1698359.

As for the problem itself, if the way we currently process the input during
> svnadmin load and load-revprops is causing a noticeable overhead, I think
> that
> we should introduce -F (--file) option to both of these commands:
> svnadmin load /path/to/repos -F (--file) /path/to/dump
> svnadmin load-revprops /path/to/repos -F (--file) /path/to/dump

I wanted to add -F for a while now because it makes
debugging much easier (when using IDEs). Never got
around it, though.

So, feel free to add -F support to load and load-revprops.

-- Stefan^2.
Received on 2015-09-02 13:50:27 CEST

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