On Tue, Sep 1, 2015 at 7:26 PM, Evgeny Kotkov <evgeny.kot...@visualsvn.com> wrote:
> Stefan Fuhrmann <stefan.fuhrm...@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.
