You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Branko Čibej <br...@wandisco.com> on 2015/09/07 09:30:07 UTC
Re: svn commit: r1701564 -
/subversion/trunk/subversion/libsvn_subr/stream.c
On 07.09.2015 09:15, ivan@apache.org wrote:
> Author: ivan
> Date: Mon Sep 7 07:15:25 2015
> New Revision: 1701564
>
> URL: http://svn.apache.org/r1701564
> Log:
> Reduce difference between Windows and non-Windows implementation of
> install_stream.
>
> * subversion/libsvn_subr/stream.c
> (install_close): Remove #ifdef WIN32
> (svn_stream__create_for_install): Always setup flush on close stream
> handler.
> (svn_stream__install_stream): Close temporary file before rename on all
> platforms.
> (svn_stream__install_get_info): Obtain file info from file handle on all
> platforms.
> (svn_stream__install_delete): Close temporary file before delete on all
> platforms.
Why do you think this is a good idea? I know you can't move/delete an
open file on Windows without jumping through hoops that APR doesn't
provide, but you certainly can do that on Unix and if memory serves,
that actually helps performance in some cases where remote file systems
are involved.
-- Brane
Re: svn commit: r1701564 - /subversion/trunk/subversion/libsvn_subr/stream.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 7 September 2015 at 12:25, Branko Čibej <br...@wandisco.com> wrote:
> On 07.09.2015 10:52, Ivan Zhakov wrote:
>> On 7 September 2015 at 11:46, Branko Čibej <br...@wandisco.com> wrote:
>>> On 07.09.2015 09:38, Ivan Zhakov wrote:
>>>> On 7 September 2015 at 10:30, Branko Čibej <br...@wandisco.com> wrote:
>>>>> On 07.09.2015 09:15, ivan@apache.org wrote:
>>>>>> Author: ivan
>>>>>> Date: Mon Sep 7 07:15:25 2015
>>>>>> New Revision: 1701564
>>>>>>
>>>>>> URL: http://svn.apache.org/r1701564
>>>>>> Log:
>>>>>> Reduce difference between Windows and non-Windows implementation of
>>>>>> install_stream.
>>>>>>
>>>>>> * subversion/libsvn_subr/stream.c
>>>>>> (install_close): Remove #ifdef WIN32
>>>>>> (svn_stream__create_for_install): Always setup flush on close stream
>>>>>> handler.
>>>>>> (svn_stream__install_stream): Close temporary file before rename on all
>>>>>> platforms.
>>>>>> (svn_stream__install_get_info): Obtain file info from file handle on all
>>>>>> platforms.
>>>>>> (svn_stream__install_delete): Close temporary file before delete on all
>>>>>> platforms.
>>>>> Why do you think this is a good idea? I know you can't move/delete an
>>>>> open file on Windows without jumping through hoops that APR doesn't
>>>>> provide, but you certainly can do that on Unix and if memory serves,
>>>>> that actually helps performance in some cases where remote file systems
>>>>> are involved.
>>>>>
>>>> Before this patch temporary file was closed anyway before
>>>> rename/delete in svn_stream_close() handler for temporary file on
>>>> non-Windows platform. Windows specific code postponed this to
>>>> rename/delete stage. The r1701564 makes this code the same on Windows
>>>> and non-Windows.
>>> Yes, I understand that; what I don't understand is why you think this
>>> change was necessary. AFAIK it just causes a slight performance hit on
>>> Unix platforms with remote file systems.
>>>
>> What change do you mean? The actual operations performed on unix
>> didn't changed in this commit (except obtaining finfo from handle
>> instead of by path), only the code changed. Before this change
>> temporary files was closed in svn_stream_close() which is called
>> before svn_stream__install_stream/svn_stream__install_delete. After
>> this change it's performed in
>> svn_stream__install_stream/svn_stream__install_delete itself before
>> rename/delete.
>>
>> What performance hit do you mean?
>
>
> Looks like I'm stupid today and managed to read the diff the wrong way
> around.
> Sorry ... just ignore me until I get my head screwed on correctly again.
>
No problem. Diff sometimes is very confusing :)
--
Ivan Zhakov
Re: svn commit: r1701564 -
/subversion/trunk/subversion/libsvn_subr/stream.c
Posted by Branko Čibej <br...@wandisco.com>.
On 07.09.2015 10:52, Ivan Zhakov wrote:
> On 7 September 2015 at 11:46, Branko Čibej <br...@wandisco.com> wrote:
>> On 07.09.2015 09:38, Ivan Zhakov wrote:
>>> On 7 September 2015 at 10:30, Branko Čibej <br...@wandisco.com> wrote:
>>>> On 07.09.2015 09:15, ivan@apache.org wrote:
>>>>> Author: ivan
>>>>> Date: Mon Sep 7 07:15:25 2015
>>>>> New Revision: 1701564
>>>>>
>>>>> URL: http://svn.apache.org/r1701564
>>>>> Log:
>>>>> Reduce difference between Windows and non-Windows implementation of
>>>>> install_stream.
>>>>>
>>>>> * subversion/libsvn_subr/stream.c
>>>>> (install_close): Remove #ifdef WIN32
>>>>> (svn_stream__create_for_install): Always setup flush on close stream
>>>>> handler.
>>>>> (svn_stream__install_stream): Close temporary file before rename on all
>>>>> platforms.
>>>>> (svn_stream__install_get_info): Obtain file info from file handle on all
>>>>> platforms.
>>>>> (svn_stream__install_delete): Close temporary file before delete on all
>>>>> platforms.
>>>> Why do you think this is a good idea? I know you can't move/delete an
>>>> open file on Windows without jumping through hoops that APR doesn't
>>>> provide, but you certainly can do that on Unix and if memory serves,
>>>> that actually helps performance in some cases where remote file systems
>>>> are involved.
>>>>
>>> Before this patch temporary file was closed anyway before
>>> rename/delete in svn_stream_close() handler for temporary file on
>>> non-Windows platform. Windows specific code postponed this to
>>> rename/delete stage. The r1701564 makes this code the same on Windows
>>> and non-Windows.
>> Yes, I understand that; what I don't understand is why you think this
>> change was necessary. AFAIK it just causes a slight performance hit on
>> Unix platforms with remote file systems.
>>
> What change do you mean? The actual operations performed on unix
> didn't changed in this commit (except obtaining finfo from handle
> instead of by path), only the code changed. Before this change
> temporary files was closed in svn_stream_close() which is called
> before svn_stream__install_stream/svn_stream__install_delete. After
> this change it's performed in
> svn_stream__install_stream/svn_stream__install_delete itself before
> rename/delete.
>
> What performance hit do you mean?
Looks like I'm stupid today and managed to read the diff the wrong way
around.
Sorry ... just ignore me until I get my head screwed on correctly again.
-- Brane
Re: svn commit: r1701564 - /subversion/trunk/subversion/libsvn_subr/stream.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 7 September 2015 at 11:46, Branko Čibej <br...@wandisco.com> wrote:
> On 07.09.2015 09:38, Ivan Zhakov wrote:
>> On 7 September 2015 at 10:30, Branko Čibej <br...@wandisco.com> wrote:
>>> On 07.09.2015 09:15, ivan@apache.org wrote:
>>>> Author: ivan
>>>> Date: Mon Sep 7 07:15:25 2015
>>>> New Revision: 1701564
>>>>
>>>> URL: http://svn.apache.org/r1701564
>>>> Log:
>>>> Reduce difference between Windows and non-Windows implementation of
>>>> install_stream.
>>>>
>>>> * subversion/libsvn_subr/stream.c
>>>> (install_close): Remove #ifdef WIN32
>>>> (svn_stream__create_for_install): Always setup flush on close stream
>>>> handler.
>>>> (svn_stream__install_stream): Close temporary file before rename on all
>>>> platforms.
>>>> (svn_stream__install_get_info): Obtain file info from file handle on all
>>>> platforms.
>>>> (svn_stream__install_delete): Close temporary file before delete on all
>>>> platforms.
>>>
>>> Why do you think this is a good idea? I know you can't move/delete an
>>> open file on Windows without jumping through hoops that APR doesn't
>>> provide, but you certainly can do that on Unix and if memory serves,
>>> that actually helps performance in some cases where remote file systems
>>> are involved.
>>>
>> Before this patch temporary file was closed anyway before
>> rename/delete in svn_stream_close() handler for temporary file on
>> non-Windows platform. Windows specific code postponed this to
>> rename/delete stage. The r1701564 makes this code the same on Windows
>> and non-Windows.
>
> Yes, I understand that; what I don't understand is why you think this
> change was necessary. AFAIK it just causes a slight performance hit on
> Unix platforms with remote file systems.
>
What change do you mean? The actual operations performed on unix
didn't changed in this commit (except obtaining finfo from handle
instead of by path), only the code changed. Before this change
temporary files was closed in svn_stream_close() which is called
before svn_stream__install_stream/svn_stream__install_delete. After
this change it's performed in
svn_stream__install_stream/svn_stream__install_delete itself before
rename/delete.
What performance hit do you mean?
--
Ivan Zhakov
Re: svn commit: r1701564 -
/subversion/trunk/subversion/libsvn_subr/stream.c
Posted by Branko Čibej <br...@wandisco.com>.
On 07.09.2015 09:38, Ivan Zhakov wrote:
> On 7 September 2015 at 10:30, Branko Čibej <br...@wandisco.com> wrote:
>> On 07.09.2015 09:15, ivan@apache.org wrote:
>>> Author: ivan
>>> Date: Mon Sep 7 07:15:25 2015
>>> New Revision: 1701564
>>>
>>> URL: http://svn.apache.org/r1701564
>>> Log:
>>> Reduce difference between Windows and non-Windows implementation of
>>> install_stream.
>>>
>>> * subversion/libsvn_subr/stream.c
>>> (install_close): Remove #ifdef WIN32
>>> (svn_stream__create_for_install): Always setup flush on close stream
>>> handler.
>>> (svn_stream__install_stream): Close temporary file before rename on all
>>> platforms.
>>> (svn_stream__install_get_info): Obtain file info from file handle on all
>>> platforms.
>>> (svn_stream__install_delete): Close temporary file before delete on all
>>> platforms.
>>
>> Why do you think this is a good idea? I know you can't move/delete an
>> open file on Windows without jumping through hoops that APR doesn't
>> provide, but you certainly can do that on Unix and if memory serves,
>> that actually helps performance in some cases where remote file systems
>> are involved.
>>
> Before this patch temporary file was closed anyway before
> rename/delete in svn_stream_close() handler for temporary file on
> non-Windows platform. Windows specific code postponed this to
> rename/delete stage. The r1701564 makes this code the same on Windows
> and non-Windows.
Yes, I understand that; what I don't understand is why you think this
change was necessary. AFAIK it just causes a slight performance hit on
Unix platforms with remote file systems.
-- Brane
Re: svn commit: r1701564 - /subversion/trunk/subversion/libsvn_subr/stream.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 7 September 2015 at 10:30, Branko Čibej <br...@wandisco.com> wrote:
> On 07.09.2015 09:15, ivan@apache.org wrote:
>> Author: ivan
>> Date: Mon Sep 7 07:15:25 2015
>> New Revision: 1701564
>>
>> URL: http://svn.apache.org/r1701564
>> Log:
>> Reduce difference between Windows and non-Windows implementation of
>> install_stream.
>>
>> * subversion/libsvn_subr/stream.c
>> (install_close): Remove #ifdef WIN32
>> (svn_stream__create_for_install): Always setup flush on close stream
>> handler.
>> (svn_stream__install_stream): Close temporary file before rename on all
>> platforms.
>> (svn_stream__install_get_info): Obtain file info from file handle on all
>> platforms.
>> (svn_stream__install_delete): Close temporary file before delete on all
>> platforms.
>
>
> Why do you think this is a good idea? I know you can't move/delete an
> open file on Windows without jumping through hoops that APR doesn't
> provide, but you certainly can do that on Unix and if memory serves,
> that actually helps performance in some cases where remote file systems
> are involved.
>
Before this patch temporary file was closed anyway before
rename/delete in svn_stream_close() handler for temporary file on
non-Windows platform. Windows specific code postponed this to
rename/delete stage. The r1701564 makes this code the same on Windows
and non-Windows.
--
Ivan Zhakov