RE: svn commit: r1003924 - /subversion/trunk/subversion/svnrdump/dump_editor.c

[Bert Huijben <be...@qqmail.nl>]

> -----Original Message-----
> From: artagnon@apache.org [mailto:artagnon@apache.org]
> Sent: zondag 3 oktober 2010 9:05
> To: commits@subversion.apache.org
> Subject: svn commit: r1003924 -
> /subversion/trunk/subversion/svnrdump/dump_editor.c
> 
> Author: artagnon
> Date: Sun Oct  3 07:05:05 2010
> New Revision: 1003924
> 
> URL: http://svn.apache.org/viewvc?rev=1003924&view=rev
> Log:
> svnrdump: dump_editor: Use just one temporary file for all textdelta
> application by truncating and re-using it in each iteration. This has
> significant performance benefits.
> 
> * subversion/svnrdump/dump_editor.c
> 
>   (dump_edit_baton): Correct the comment about the description of
>   pool, add a new delta_file variable and note in a comment that
>   delta_file and delta_abspath should be allocated in the
>   per-edit-session pool, not in the per-revision pool like the other
>   variables.
> 
>   (get_dump_editor): Open the delta file initializing `eb->delta_file`
>   and `eb->delta_abspath`, allocating them in the per-edit-session
>   pool. Mark delta_file as a del_on_close, so that it's automatically
>   cleaned up when the editor is done. All streams in callbacks that
>   map to it should set disown = TRUE.
> 
>   (close_file): Instead of opening the delta_file, simply seek to the
>   beginning. During cleanup, don't close or remove the file; just
>   truncate it to get it ready for the next textdelta
>   application. Also, while getting the file stat, don't re-open the
>   file -- use apr_file_info_get on the open file instead.
> 
>   (apply_textdelta): Don't create a unique file. Simply map a stream
>   to the delta file to perform the textdelta application. Disown the
>   stream and close it explicitly, making sure not to close the delta
>   file itself.
> 
> Patch by: David Barr <da...@cordelta.com>
> Helped by: artagnon
> 

Looks good; nice optimization :)

> 
> @@ -846,7 +853,14 @@ get_dump_editor(const svn_delta_editor_t
> 
>    /* Create a special per-revision pool */
>    eb->pool = svn_pool_create(pool);
> -
> +
> +  /* Open a unique temporary file for all textdelta applications in
> +     this edit session. The file is automatically closed and cleaned
> +     up when the edit session is done. */
> +  SVN_ERR(svn_io_open_uniquely_named(&(eb->delta_file), &(eb-
> >delta_abspath),
> +                                     NULL, "svnrdump", NULL,
> +                                     svn_io_file_del_on_close, pool,
> pool));

(This code is probably copied from the old code; but anyway:)

Do you really need a 'named' file here?

The named file algorithm of svn_io_open_uniquely_named() is very slow compared to the 'just get me a temporary file' implemented by svn_io_open_unique_file3(). Especially when you need more than a few tempfiles with the same basename. (It tries the generated name itself, then it tries a 1 suffix, a 2 suffix, etc. Until it finds a name that doesn't exist)

	Bert 

Re: svn commit: r1003924 - /subversion/trunk/subversion/svnrdump/dump_editor.c

[Ramkumar Ramachandra <ar...@gmail.com>]

Hi Bert,

Bert Huijben writes:
> Do you really need a 'named' file here?
> 
> The named file algorithm of svn_io_open_uniquely_named() is very slow compared to the 'just get me a temporary file' implemented by svn_io_open_unique_file3(). Especially when you need more than a few tempfiles with the same basename. (It tries the generated name itself, then it tries a 1 suffix, a 2 suffix, etc. Until it finds a name that doesn't exist)

Ah, I see. The named temporary made debugging easier initially- I
could easily see if the file was being removed or not, but I suppose
it doesn't matter anymore. Fixed in r1003958. Thanks for the
suggestion.

-- Ram