On Tue, Sep 1, 2015 at 7:26 PM, Evgeny Kotkov <evgeny.kotkov_at_visualsvn.com>
wrote:
> 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
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.
Received on 2015-09-02 13:50:27 CEST