You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by iv...@apache.org on 2015/09/07 09:15:25 UTC
svn commit: r1701564 - /subversion/trunk/subversion/libsvn_subr/stream.c
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.
Modified:
subversion/trunk/subversion/libsvn_subr/stream.c
Modified: subversion/trunk/subversion/libsvn_subr/stream.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/stream.c?rev=1701564&r1=1701563&r2=1701564&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_subr/stream.c (original)
+++ subversion/trunk/subversion/libsvn_subr/stream.c Mon Sep 7 07:15:25 2015
@@ -2181,6 +2181,8 @@ create_tempfile(HANDLE *hFile,
return SVN_NO_ERROR;
}
+#endif /* WIN32 */
+
/* Implements svn_close_fn_t */
static svn_error_t *
install_close(void *baton)
@@ -2193,8 +2195,6 @@ install_close(void *baton)
return SVN_NO_ERROR;
}
-#endif /* WIN32 */
-
svn_error_t *
svn_stream__create_for_install(svn_stream_t **install_stream,
const char *tmp_abspath,
@@ -2249,12 +2249,8 @@ svn_stream__create_for_install(svn_strea
ib->tmp_path = tmp_path;
-#ifdef WIN32
/* Don't close the file on stream close; flush instead */
svn_stream_set_close(*install_stream, install_close);
-#else
- /* ### Install pool cleanup handler for tempfile? */
-#endif
return SVN_NO_ERROR;
}
@@ -2299,8 +2295,6 @@ svn_stream__install_stream(svn_stream_t
svn_io_file_rename2(). */
svn_error_clear(err);
err = SVN_NO_ERROR;
-
- SVN_ERR(svn_io_file_close(ib->baton_apr.file, scratch_pool));
}
else
{
@@ -2310,6 +2304,9 @@ svn_stream__install_stream(svn_stream_t
}
#endif
+ /* Close temporary file. */
+ SVN_ERR(svn_io_file_close(ib->baton_apr.file, scratch_pool));
+
err = svn_io_file_rename2(ib->tmp_path, final_abspath, FALSE, scratch_pool);
/* A missing directory is too common to not cover here. */
@@ -2343,19 +2340,12 @@ svn_stream__install_get_info(apr_finfo_t
apr_pool_t *scratch_pool)
{
struct install_baton_t *ib = install_stream->baton;
-
-#ifdef WIN32
- /* On WIN32 the file is still open, so we can obtain the information
- from the handle without race conditions */
apr_status_t status;
status = apr_file_info_get(finfo, wanted, ib->baton_apr.file);
if (status)
return svn_error_wrap_apr(status, NULL);
-#else
- SVN_ERR(svn_io_stat(finfo, ib->tmp_path, wanted, scratch_pool));
-#endif
return SVN_NO_ERROR;
}
@@ -2382,9 +2372,10 @@ svn_stream__install_delete(svn_stream_t
/* Deleting file on close may be unsupported, so ignore errors and
fallback to svn_io_remove_file2(). */
svn_error_clear(err);
- SVN_ERR(svn_io_file_close(ib->baton_apr.file, scratch_pool));
#endif
+ SVN_ERR(svn_io_file_close(ib->baton_apr.file, scratch_pool));
+
return svn_error_trace(svn_io_remove_file2(ib->tmp_path, FALSE,
scratch_pool));
}
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
Re: svn commit: r1701564 -
/subversion/trunk/subversion/libsvn_subr/stream.c
Posted by Branko Čibej <br...@wandisco.com>.
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