patchmacos: lazily initialize iconv

> In practice, the majority of paths do not have any utf8 character
> that needs the canonicalization.  Lazily call iconv_open() and
> iconv_close() to avoid unnecessary overhead.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>
>  * This is not even compile tested, so it needs testing and
>    benchmarking, as I do not even know how costly the calls to
>    open/close are when we do not have to call iconv() itself.
>
>    This was brought up by Linus (Cc'ed) in http://goo.gl/INWVc

> +       /* Avoid iconv_open()/iconv_close() if there is nothing to convert */
> +       for (i = 0; i < argc; i++) {
> +               if (has_utf8(argv[i], (size_t)-1, NULL))
> +                       break;
> +       }
> +       if (i < argc)
> +               return;
> +

> On Tue, Jul 31, 2012 at 7:52 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> +       /* Avoid iconv_open()/iconv_close() if there is nothing to convert */
>> +       for (i = 0; i < argc; i++) {
>> +               if (has_utf8(argv[i], (size_t)-1, NULL))
>> +                       break;
>> +       }
>> +       if (i < argc)
>> +               return;
>> +
>
> I'm not very familiar with this code but:
>
> before: it reencodes everything which is utf-8 in argv
> after: it reencodes *nothing* if one string in argv is not in utf-8
>
> am i wrong?

> +       /* Avoid iconv_open()/iconv_close() if there is nothing to convert */
> +       for (i = 0; i < argc; i++) {
> +               if (has_utf8(argv[i], (size_t)-1, NULL))
> +                       break;
> +       }
> +       if (argc <= i)
> +               return; /* no utf8 found */

> On Tue, Jul 31, 2012 at 8:37 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> +       /* Avoid iconv_open()/iconv_close() if there is nothing to convert */
>> +       for (i = 0; i < argc; i++) {
>> +               if (has_utf8(argv[i], (size_t)-1, NULL))
>> +                       break;
>> +       }
>> +       if (argc <= i)
>> +               return; /* no utf8 found */
>
> sorry, but "argc" can never be smaller than "i", right?

> On Tue, Jul 31, 2012 at 11:37 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>>  * This is not even compile tested, so it needs testing and
>>    benchmarking, as I do not even know how costly the calls to
>>    open/close are when we do not have to call iconv() itself.
>
> Ok, so it's easily compile-tested: just add
>
> +       COMPAT_OBJS += compat/precompose_utf8.o
> +       BASIC_CFLAGS += -DPRECOMPOSE_UNICODE
>
> to the makefile for Linux too.
>
> Actually testing how well it *works* is hard, since I don't really
> have a mac (well, I do, but it no longer has OS X on it ;), and the
> whole "utf-8-mac" thing does not make sense.

> HOWEVER. I actually tested it with the conversion being from Latin1 to
> UTF-8 instead, and it does interesting things, and kind of works. I
> say "kind of", because for the case of the filesystem being in Latin1,
> we actually have to convert things back to the filesystem character
> set ...

> Not just write_entry() codepath that creates the final paths on the
> filesystem, you would need to touch lstat() calls that check the
> existence and freshness of the path, once you go that route.  I am
> sure such a change can be made to work, but I am not sure how much
> we would gain from one.

> On Tue, Jul 31, 2012 at 1:16 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Eek.
>
> Oh, I agree. Doing a full character set conversion both ways is quite
> a bit more work.
>
>> Not just write_entry() codepath that creates the final paths on the
>> filesystem, you would need to touch lstat() calls that check the
>> existence and freshness of the path, once you go that route.  I am
>> sure such a change can be made to work, but I am not sure how much
>> we would gain from one.
>
> I think it might be interesting. I doubt it matters all that much any
> more in Western Europe (Unicode really does seem to have largely taken
> over), but I think Japan still uses Shift-JIS a lot.
>
> Although maybe that is starting to fade too.
>
> And it really is just a generalization of the OS X filesystem damage.
>
>              Linus
>

> In practice, the majority of paths do not have utf8 that needs
> the canonicalization. Lazily call iconv_open()/iconv_close() to
> avoid unnecessary overhead.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> Helped-by: Ralf Thielow <ralf.thielow@gmail.com>
> Helped-by: Linus Torvalds <torvalds@linux-foundation.org>
> ---
>
>  * This is not even compile tested, so it needs testing and
>    benchmarking, as I do not even know how costly the calls to
>    open/close are when we do not have to call iconv() itself.
>
>    This reroll corrects an obvious thinko pointed out by Ralf, and
>    gets rid of an extra iconv_opened field added unnecessarily in
>    the previous round.
>
>    This was brought up by Linus in http://goo.gl/INWVc
>
>  compat/precompose_utf8.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/compat/precompose_utf8.c b/compat/precompose_utf8.c
> index d40d1b3..79b5528 100644
> --- a/compat/precompose_utf8.c
> +++ b/compat/precompose_utf8.c
> @@ -67,7 +67,7 @@ void probe_utf8_pathname_composition(char *path, int len)
>  
>  void precompose_argv(int argc, const char **argv)
>  {
> -	int i = 0;
> +	int i;
>  	const char *oldarg;
>  	char *newarg;
>  	iconv_t ic_precompose;
> @@ -75,11 +75,19 @@ void precompose_argv(int argc, const char **argv)
>  	if (precomposed_unicode != 1)
>  		return;
>  
> +	/* Avoid iconv_open()/iconv_close() if there is nothing to convert */
> +	for (i = 0; i < argc; i++) {
> +		if (has_utf8(argv[i], (size_t)-1, NULL))
> +			break;
> +	}
> +	if (argc <= i)
> +		return; /* no utf8 found */
> +
>  	ic_precompose = iconv_open(repo_encoding, path_encoding);
>  	if (ic_precompose == (iconv_t) -1)
>  		return;
>  
> -	while (i < argc) {
> +	for (i = 0; i < argc; i++) {
>  		size_t namelen;
>  		oldarg = argv[i];
>  		if (has_utf8(oldarg, (size_t)-1, &namelen)) {
> @@ -87,7 +95,6 @@ void precompose_argv(int argc, const char **argv)
>  			if (newarg)
>  				argv[i] = newarg;
>  		}
> -		i++;
>  	}
>  	iconv_close(ic_precompose);
>  }
> @@ -106,8 +113,7 @@ PREC_DIR *precompose_utf8_opendir(const char *dirname)
>  		return NULL;
>  	} else {
>  		int ret_errno = errno;
> -		prec_dir->ic_precompose = iconv_open(repo_encoding, path_encoding);
> -		/* if iconv_open() fails, die() in readdir() if needed */
> +		prec_dir->ic_precompose = (iconv_t)-1;
>  		errno = ret_errno;
>  	}
>  
> @@ -136,6 +142,9 @@ struct dirent_prec_psx *precompose_utf8_readdir(PREC_DIR *prec_dir)
>  		prec_dir->dirent_nfc->d_type = res->d_type;
>  
>  		if ((precomposed_unicode == 1) && has_utf8(res->d_name, (size_t)-1, NULL)) {
> +			if (prec_dir->ic_precompose == (iconv_t)-1)
> +				prec_dir->ic_precompose =
> +					iconv_open(repo_encoding, path_encoding);
>  			if (prec_dir->ic_precompose == (iconv_t)-1) {
>  				die("iconv_open(%s,%s) failed, but needed:\n"
>  						"    precomposed unicode is not supported.\n"

> On 31.07.12 20:37, Junio C Hamano wrote:
>> In practice, the majority of paths do not have utf8 that needs
>> the canonicalization. Lazily call iconv_open()/iconv_close() to
>> avoid unnecessary overhead.
>>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> Helped-by: Ralf Thielow <ralf.thielow@gmail.com>
>> Helped-by: Linus Torvalds <torvalds@linux-foundation.org>
>> ---
>>
>>  * This is not even compile tested, so it needs testing and
>>    benchmarking, as I do not even know how costly the calls to
>>    open/close are when we do not have to call iconv() itself.
>> ...
> Hi Junio,
>
> thanks for the optimization.
> Tested-by: Torsten Bögershausen <tboegi@web.de>

> Torsten Bögershausen <tboegi@web.de> writes:
>
>> On 31.07.12 20:37, Junio C Hamano wrote:
>>> In practice, the majority of paths do not have utf8 that needs
>>> the canonicalization. Lazily call iconv_open()/iconv_close() to
>>> avoid unnecessary overhead.
>>>
>>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>>> Helped-by: Ralf Thielow <ralf.thielow@gmail.com>
>>> Helped-by: Linus Torvalds <torvalds@linux-foundation.org>
>>> ---
>>>
>>>  * This is not even compile tested, so it needs testing and
>>>    benchmarking, as I do not even know how costly the calls to
>>>    open/close are when we do not have to call iconv() itself.
>>> ...
>> Hi Junio,
>>
>> thanks for the optimization.
>> Tested-by: Torsten Bögershausen <tboegi@web.de>
> Well, I didn't mean the correctness testing without numbers.  The
> correctness of the patch after a couple of people eyeballed it was
> no longer a question.
>
> If the patch does not give any measuable performance difference to
> people who exercise this codepath, it is not worth merging.  And
> that is not something I can't do myself without a Mac (nor I wish to
> have one to be able to do so myself).
>
> Thanks.

> After cloning, I set "git config core.precomposeunicode true".
>
> git is v1.7.12.rc2,
> git_git/git is with your patch.
> 
> git v1.7.12.rc2:
> tb@birne:~/projects/git/linux-2.6> for f in 1 2 3 4 5; do time git status  ; done 2>&1 | egrep "^user|^real|^sys"
> real    0m0.460s
> user    0m0.283s
> sys     0m0.176s
>
> With lazy init of iconv:
> tb@birne:~/projects/git/linux-2.6> for f in 1 2 3 4 5; do time ~/projects/git/git_git/git status  ; done 2>&1 | egrep "^user|^real|^sys"
>
> real    0m0.462s
> user    0m0.281s
> sys     0m0.177s