subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Stefan Fuhrmann <stefan.fuhrm...@wandisco.com>
Subject 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
Date Wed, 02 Sep 2015 11:50:14 GMT
On Tue, Sep 1, 2015 at 7:26 PM, Evgeny Kotkov <evgeny.kotkov@visualsvn.com>
wrote:

> Stefan Fuhrmann <stefan.fuhrmann@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
lists.

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.

Mime
View raw message