Re: [PATCH] diff-highlight: Fix broken multibyte string

[Jeff King <peff@peff.net>]

On Fri, Apr 03, 2015 at 11:19:24AM +0900, Yi, EungJun wrote:

> > I timed this one versus the existing diff-highlight. It's about 7%
> > slower. That's not great, but is acceptable to me. The String::Multibyte
> > version was a lot faster, which was nice (but I'm still unclear on
> > _why_).
> 
> I think the reason is here:
> 
> > sub split_line {
> >    local $_ = shift;
> >    return map { /$COLOR/ ? $_ : ($mbcs ? $mbcs->strsplit('', $_) : split //) }
> >           split /($COLOR)/;
> > }
> 
> I removed "*" from "split /($COLOR*)/". Actually I don't know why "*"
> was required but I need to remove it to make my patch works correctly.

Ah, OK, that makes more sense. The "*" was meant to handle the case of
multiple groups of ANSI colors in a row. But I think it should have been
"+" in that case, as we would otherwise split on the empty field, which
would mean character-by-character. And the second "split" in the map
would then be superfluous, which would break your patch (we've already
split the multi-byte characters before we even hit $mbcs->strsplit).

Kyle's patch does not care, because it tweaks the string so that normal
split works. Which means there is an easy speedup here. :)

Doing:

diff --git a/contrib/diff-highlight/diff-highlight b/contrib/diff-highlight/diff-highlight
index 08c88bb..1c4b599 100755
--- a/contrib/diff-highlight/diff-highlight
+++ b/contrib/diff-highlight/diff-highlight
@@ -165,7 +165,7 @@ sub highlight_pair {
 sub split_line {
 	local $_ = shift;
 	return map { /$COLOR/ ? $_ : (split //) }
-	       split /($COLOR*)/;
+	       split /($COLOR+)/;
 }
 
 sub highlight_line {

gives me a 25% speed improvement, and the same output processing
git.git's entire "git log -p" output.

I thought that meant we could also optimize out the "map" call entirely,
and just use the first split (with "*") to end up with a list of $COLOR
chunks and single characters, but it does not seem to work. So maybe I
am misreading something about what is going on.

-Peff