Re: [PATCH]: Optimize merge_file_trivial()

See attached the next iteration of that patch. I (hopefully) addressed all issues that were raised up to now on this list. [[[ Optimize merge_file_trivial() by avoiding to read the files twice by using a new comparison function which compares 3 files at once. Also add C tests for the new and existing file comparison functions in libsvn_subr/io.c * subversion/include/svn_io.h (svn_io_filesizes_three_different_p): Add new declaration (svn_io_files_contents_three_same_p): Add new declaration * subversion/libsvn_subr/io.c (svn_io_filesizes_three_different_p): Add new function in analogy to svn_io_filesizes_different_p(). (contents_three_identical_p): Add new function in analogy to contents_identical_p(). (svn_io_files_contents_three_same_p): Add new function in analogy to svn_io_files_contents_same_p. * subversion/libsvn_wc/merge.c (merge_file_trivial): Use the new three-file comparison functions to avoid reading files twice. * build.conf Add the new io-test.c file to the build configuration. * subversion/tests add the temporary test file folder to svn:ignore. * subversion/tests/libsvn_subr/io-test.c (create_test_file): Helper function to create a test data file. (create_comparison_candidates): Helper function to create the full set of test data files. (test_two_file_size_comparison): Test function for svn_io_filesizes_different_p. (test_two_file_content_comparison): Test function for svn_io_files_contents_same_p. (test_three_file_size_comparison): Test function for test_three_file_size_comparison. (test_three_file_content_comparison): Test function for svn_io_files_contents_three_same_p. Patch by: Markus Schaber <m.schaber@3s-software.com> ]]] I also re-ran the tests, and they still look fine: Summary of test results: 1623 tests PASSED 85 tests SKIPPED 38 tests XFAILED (1 WORK-IN-PROGRESS) Best regards Markus Schaber -- We software Automation. 3S-Smart Software Solutions GmbH Markus Schaber | Developer Memminger Str. 151 | 87439 Kempten | Germany | Tel. +49-831-54031-0 | Fax +49-831-54031-50 Email: m.schaber@3s-software.com | Web: http://www.3s-software.com CoDeSys internet forum: http://forum.3s-software.com Download CoDeSys sample projects: http://www.3s-software.com/index.shtml?sample_projects Managing Directors: Dipl.Inf. Dieter Hess, Dipl.Inf. Manfred Werner | Trade register: Kempten HRB 6186 | Tax ID No.: DE 167014915 ________________________________________ Von: Bert Huijben [bert@qqmail.nl] Gesendet: Samstag, 16. Juni 2012 07:28 Bis: 'Philip Martin'; Markus Schaber Cc: dev@subversion.apache.org Betreff: RE: AW: [PATCH]: Optimize merge_file_trivial() > -----Original Message----- > From: MARTIN PHILIP [mailto:codematters@ntlworld.com] On Behalf Of > Philip Martin > Sent: Saturday, June 16, 2012 1:00 AM > To: Markus Schaber > Cc: dev@subversion.apache.org > Subject: Re: AW: [PATCH]: Optimize merge_file_trivial() > > Markus Schaber <m.schaber@3s-software.com> writes: > > > Hi, Philip, > > > > I did address most of the issues. However, the following question is open: > > > > Von: Philip Martin [philip.martin@wandisco.com] > >> Markus Schaber <m.schaber@3s-software.com> writes: > > > >>> +/* Function to prepare a single test file */ > >>> + > >>> +static svn_error_t * > >>> +create_test_file(struct test_file_definition_t* definition, apr_pool_t > *pool, apr_pool_t *scratch_pool) > >>> +{ > >>> + apr_status_t status; > >>> + apr_file_t *file_h; > >>> + apr_off_t midpos = definition->size / 2; > >>> + svn_error_t *err = NULL; > >>> + int i; > >>> + > >>> + if (definition->size < 5) > >>> + SVN_ERR_ASSERT(strlen(definition->data) >= definition->size); > >>> + else > >>> + SVN_ERR_ASSERT(strlen(definition->data) >= 5); > >>> + > >>> + status = apr_filepath_merge(&definition->created_path, > >>> + TEST_DIR, > >>> + definition->name, > >>> + 0, > >>> + pool); > > > >> Do you need to do that? Could you simply relative paths as is done > >> subversion/tests/libsvn_diff/diff-diff3-test.c. > > > > I used path concatenation to pack all of the files into one temporary > > directory, so they can be easily cleaned up, and only one svn:ignore > > entry is needed. I did model this after how the temporary > > repositories are handled in svn_test_fs.c. > > I meant use svn_relpath_join rather than apr_filepath_merge. We > generally prefer the svn_ functions over the apr_ functions. The svn_ > functions often lead to simpler error handling because they return > svn_error_t. The svn_ IO functions also take paths in UTF-8 which > usually makes things easier since paths are generally UTF-8 in > Subversion code. This code should use svn_dirent_join() as these paths are not relpaths, but on-disk files. svn_relpath would assert in maintainer mode when passing a UNC or unix style absolute path. And several required path transformations on Windows would be ignored. Bert