A few too-wide lines corrected, and some places where clang-format
prefers to wrap after the binary operator instead of before. I don't
feel strongly, so I'm leaving the auto-format result as "after the
binary operator".
Differential Revision: https://phab.mercurial-scm.org/D1007
This is pretty much a copy of f0646ce71ed6, just to a different loop.
The condition `p == plast` (`plast == a + len - 1`) was only true on
the final iteration of the loop. So it was wasteful to check for it
on every iteration. We decrease the iteration count by 1 and add an
explicit check for `p == plast` after the loop.
Again, we see modest wins.
From the mozilla-unified repository:
$ perfbdiff -m 3041e4d59df2
! wall 0.035502 comb 0.040000 user 0.040000 sys 0.000000 (best of 100)
! wall 0.030480 comb 0.030000 user 0.030000 sys 0.000000 (best of 100)
$ perfbdiff 0e9928989e9c --alldata --count 100
! wall 4.097394 comb 4.100000 user 4.100000 sys 0.000000 (best of 3)
! wall 3.597798 comb 3.600000 user 3.600000 sys 0.000000 (best of 3)
The 2nd example throws a total of ~3.3GB of data at bdiff. This
change increases the throughput from ~811 MB/s to ~924 MB/s.
[This change could be folded into the previous changeset to minimize the repo
churn ...]
Similar to the previous change, introduce an exception to the general
preference for matches in the middle of bdiff ranges: If the best match on the
B side starts at the beginning of the bdiff range, don't aim for the
middle-most A side match but for the earliest.
New (later) matches on the A side will only be considered better if the
corresponding match on the B side *not* is at the beginning of the range.
Thus, if the best (middle-most) match on the B side turns out to be at the
beginning of the range, the earliest match on the A side will be used.
The bundle size for 4.0 (hg bundle --base null -r 4.0 x.hg) happens to go from
22807275 to 22808120 bytes - a 0.004% increase.
[This change could be folded into the previous changeset to minimize the repo
churn ...]
The general preference to matches in the middle of bdiff ranges helps getting
balanced recursion and efficient computation. But, as previous changes have
shown, it might also give diffs that seems "obviously wrong".
To mitigate that: If the best match on the A side starts at the beginning of
the bdiff range, don't aim for the middle-most B side match but for the
earliest.
This will make the matches balanced (by both sides being "early") even though
the bisection will be less balanced. Still, this case only apply if the *best*
and middle-most match was fully unbalanced on the A side. Each recursion will
thus even in this worst case reduce the problem significantly and we are not
re-introducing the problem that was fixed in d3deb406b55b.
The bundle size for 4.0 (hg bundle --base null -r 4.0 x.hg) happens to go from
22806817 to 22807275 bytes - a 0.002% increase.
This make the recent test-bdiff.py changes give a more pretty output ... but
they no longer show that the recursion is around middle matches (because it in
these cases isn't).
We already have a slight preference for matches close to the middle on the A
side. Now, do the same on the B side.
j is iterating the b range backwards and we thus accept a new j if the previous
match was in the upper half.
This makes the test-bhalf diff "correct". It obviously also gives more
preference to balanced recursion than to appending to sequences. That is kind
of correct, but will also unfortunately make some bundles bigger. No doubt, we
can also create examples where it will make them smaller ...
The bundle size for 4.0 (hg bundle --base null -r 4.0 x.hg) happens to go from
22803824 to 22806817 bytes - an 0.01% increase.
This is primarily to make the code more managable and prepare for later
changes.
More specific assignments might also be slightly faster, even thought it also
might generate a bit more code.
We prefer matches closer to the middle to balance recursion, as introduced in
d3deb406b55b.
For ranges with uneven length, matches starting exactly in the middle should
have preference. That will be optimal for matches of length 1. We will thus
accept equality in the half check.
For ranges with even length, half was ceil'ed when calculated but we got the
preference for low matches from the 'less than half' check. To get the same
result as before when we also accept equality, floor it. Without that,
test-annotate.t would show some different (still correct but less optimal)
results.
This will change the heuristics. Tests shows a slightly different output - and
sometimes slightly smaller bundles.
The bundle size for 4.0 (hg bundle --base null -r 4.0 x.hg) happens to go from
22804885 to 22803824 bytes - an 0.005% reduction.
This patch replaces lyhash with the hash algorithm used by diffutils.
The algorithm has its origins in Git commit 2e9d1410, which is all the
way back from 1992. The license header in the code at that revision
in GPL v2.
I have not performed an extensive analysis of the distribution
(and therefore buckets) of hash output. However, `hg perfbdiff`
gives some clear wins. I'd like to think that if it is good enough
for diffutils it is good enough for us?
From the mozilla-unified repository:
$ perfbdiff -m 3041e4d59df2
! wall 0.053271 comb 0.060000 user 0.060000 sys 0.000000 (best of 100)
! wall 0.035827 comb 0.040000 user 0.040000 sys 0.000000 (best of 100)
$ perfbdiff 0e9928989e9c --alldata --count 100
! wall 6.204277 comb 6.200000 user 6.200000 sys 0.000000 (best of 3)
! wall 4.309710 comb 4.300000 user 4.300000 sys 0.000000 (best of 3)
From the hg repo:
$ perfbdiff 35000 --alldata --count 1000
! wall 0.660358 comb 0.660000 user 0.660000 sys 0.000000 (best of 15)
! wall 0.534092 comb 0.530000 user 0.530000 sys 0.000000 (best of 19)
Looking at the generated assembly and statistical profiler output
from the kernel level, I believe there is room to make this function
even faster. Namely, we're still consuming data character by character
instead of at the word level. This translates to more loop iterations
and more instructions.
At this juncture though, the real performance killer is that we're
hashing every line. We should get a significant speedup if we change
the algorithm to find the longest prefix, longest suffix, treat those
as single "lines" and then only do the line splitting and hashing on
the parts that are different. That will require a lot of C code,
however. I'm optimistic this approach could result in a ~2x speedup.
`plast = a + len - 1`. So, this "for" loop iterates from "a" to "plast",
inclusive. So, `p == plast` can only be true on the final iteration
of the loop. So checking for it on every loop iteration is wasteful.
This patch simply decreases the upper bound of the loop by 1 and
adds an explicit check after iteration for the `p == plast` case.
We can't simply add 1 to the initial value for "i" because that
doesn't do the correct thing on empty input strings.
`perfbdiff -m 3041e4d59df2` on the Firefox repo becomes significantly
faster:
! wall 0.072763 comb 0.070000 user 0.070000 sys 0.000000 (best of 100)
! wall 0.053221 comb 0.060000 user 0.060000 sys 0.000000 (best of 100)
For the curious, this code has its origins in 3605ecc924ef, which is
the changeset that introduced bdiff.c in 2005.
Also, GNU diffutils is able to perform a similar line-based diff in
under 20ms. So there's likely more perf wins to be found in this code.
One of them is the hashing algorithm. But it looks like mpm spent
some time testing hash collisions in c22788816627. I'd like to do the
same before switching away from lyhash, just to be on the safe side.
Now that we extend matches backwards in the inner loop, the final
adjustment has no effect.
(A similar extension for the forward direction is trickier and has
less benefit.)
For very large diffs that have large numbers of identical lines (JSON
dumps) that also have large blocks of identical text, bdiff could become
confused about which block matches which because it can only match
very limited regions. The result is very large diffs for small sets of edits.
The earlier recursion rebalancing fix made this behavior more frequent because
it's now more prone to match block 1 to block 2. One frequent user of
large JSON files reported being unable to pass the resulting diffs
through their code review system.
Prior to this change, bdiff would calculate the length of a match at
(i, j) as 1 + length found at (i-1, j-1). With large number of popular
(ignored) lines, this often meant matches couldn't be extended
backwards at all and thus all matching regions were very small.
Disabling the popularity threshold is not an option because it brings
back quadratic behavior.
Instead, we extend a match backwards until we either found a previously
discovered match or we find a mismatching line. This thus successfully
bridges over any popular lines inside and before a matching region.
The larger regions then significant reduce the probability of confusion.
This causes the longest_match search to limit itself to a window of
30000 lines during search (roughly 1MB), thus avoiding a full O(N*M)
search that might occur in repetitive structured inputs. For a
particular class of many MB pathological test cases, this generated
the following timings:
size before after
10x 1.25s 1.24s
100x 57s 33s
1000x >8400s 400s
The times on the right quickly become much faster and appear more linear.
While windowing means deltas are no longer "optimal", the resulting
deltas were within a couple percent of expected size. While we've yet
to have a report of a file with the level of repetition necessary to
hit this case, some JSON/XML database dump scenario is fairly likely
to hit it.
This may also slightly improve the average-case performance for deltas
of large binaries.
For highly structured files like JSON or XML dumps with large numbers
of duplicate lines (eg braces) and isolated matching lines, bdiff
could find large numbers of equally good spans. Because it prefers
earlier matches, this would result in pathologically unbalance
recursion that resulted in quadratic performance.
This patch makes it prefer matches closer to the middle that tend to
balance recursion. This change improves the speed of a pathological
test case from 1100s to 9s.
Included is a smaller test that has a roughly 50x safety margin on the
performance it accepts. It's likely to fail on pure builds because
difflib also has a recursion-balancing problem.
The longest_match code compares all the possible positions in two
files to find the best match. Given a pair of sequences, it
effectively searches a grid like this:
a b b b c . d e . f
0 1 2 3 4 5 6 7 8 9
a 1 - - - - - - - - -
b - 2 1 1 - - - - - -
b - 1 3 2 - - - - - -
b - 1 2 4 - - - - - -
. - - - - - 1 - - 1 -
Here, the 4 in the middle says "the first four lines of the
file match", which it can compute be comparing the fourth lines and
then adding one to the result found when comparing the third lines in
the entry to the upper left.
We generally avoid the quadratic worst case by only looking at lines
that match, which is precomputed. We also avoid quadratic storage by
only keeping a single column vector and then keeping track of the best
match.
Unfortunately, this can get us into trouble with the sequences above.
Because we want to reuse the '3' value when calculating the '4', we
need to be careful not to overwrite it with the '2' we calculate
immediately before. If we scan left to right, top to bottom, we're
going to have a problem: we'll overwrite our 3 before we use it and
calculate a suboptimal best match.
To address this, we can either keep two column vectors and swap
between them (which significantly complicates bookkeeping), or change
our scanning order. If we instead scan from left to right, bottom to
top, we'll avoid ever overwriting values we'll need in the future.
This unfortunately needs several changes to be made simultaneously:
- change the order we build the initial hash chains for the b sequence
- change the sentinel values from INT_MAX to -1
- change the visit order in the longest_match inner loop
- add a tie-breaker preference for earlier matches
This last is needed because we previously had an implicit tie-breaker
from our visitation order that our test suite relies on. Later matches
can also trigger a bug in the normalization code in diff().
This bug is hidden by the current bias towards matches at the
beginning of the file. When this bias is tweaked later to address
recursion balancing, the normalization code could cause the next block
to shrink to a negative length, thus creating invalid delta chunks. We
add checks here to disallow that.
This bug requires test cases that are an awkwardly large size for the test
suite, but is very rapidly picked up by the included torture tester.
This just makes the code harder to read without any performance
advantage. We're going to make the check here more complex, let's make
it simpler first.
Eliminates
mercurial/bdiff.c(383) : warning C4244: 'function' : conversion from '__int64'
to 'uint32_t', possible loss of data
mercurial/bdiff.c(384) : warning C4244: 'function' : conversion from '__int64'
to 'uint32_t', possible loss of data
mercurial/bdiff.c(385) : warning C4244: 'function' : conversion from
'Py_ssize_t' to 'uint32_t', possible loss of data
when compiling for Windows x64 target using the Microsoft compiler.
Reduces the conversion warnings
mercurial/bdiff.c(61) : warning C4244: '=' : conversion from '__int64' to
'int', possible loss of data
mercurial/bdiff.c(307) : warning C4244: 'function' : conversion from
'Py_ssize_t' to 'int', possible loss of data
mercurial/bdiff.c(308) : warning C4244: 'function' : conversion from
'Py_ssize_t' to 'int', possible loss of data
mercurial/bdiff.c(362) : warning C4244: '+=' : conversion from '__int64' to
'int', possible loss of data
mercurial/bdiff.c(380) : warning C4244: '=' : conversion from '__int64' to
'int', possible loss of data
mercurial/bdiff.c(381) : warning C4244: 'function' : conversion from '__int64'
to 'uint32_t', possible loss of data
mercurial/bdiff.c(382) : warning C4244: 'function' : conversion from '__int64'
to 'uint32_t', possible loss of data
mercurial/bdiff.c(416) : warning C4244: '=' : conversion from 'Py_ssize_t' to
'int', possible loss of data
to
mercurial/bdiff.c(383) : warning C4244: 'function' : conversion from '__int64'
to 'uint32_t', possible loss of data
mercurial/bdiff.c(384) : warning C4244: 'function' : conversion from '__int64'
to 'uint32_t', possible loss of data
mercurial/bdiff.c(385) : warning C4244: 'function' : conversion from
'Py_ssize_t' to 'uint32_t', possible loss of data
on the three putbe32() calls in the function bdiff
when compiling for Windows x64 target using the Microsoft compiler.
This means that threaded webservers will have more of a chance of
doing something useful while the C extension is busy computing a
delta. Not doing this was causing problems for Google Code with a 25
meg text file that takes O(7 minutes) to deltify.
If fixws() is called on a zero-length string, malloc(0) is called and
expected to return a pointer. Which it does on e.g. Linux. AIX returns
NULL, which it is also legal, but the malloc() is then assumed to have
failed. So ensure a valid pointer is always returned.
This patch adds support for py3k in bdiff.c. This is accomplished by including
a header file responsible for abstracting the API differences between python 2
and python 3.
Patch from Jason Orendorff
The lower the threshold, the stronger the popularity hack's
influence. So at 3999 lines, the hack is disabled; and at 4000 lines,
the hack is enabled at maximum strength (t=4).
No source file in mercurial/crew is over 4000 lines. But there are, oh,
a few such files in Mozilla. I can testify that this hack causes hg to
generate some correct but eyebrow-raising patches.
I think the hack should phase in gradually. The threshold should be high
for small files where we don't need it so much. Like this:
t = (bn < 31000) ? 1000000 / bn : bn / 1000;
That would leave the popularity hack disabled for small files, then
gradually phase it in:
bn < 1000 -- t > bn (popularity hack is completely disabled)
bn == 1000 -- t = 1000 (still effectively disabled)
bn == 2000 -- t = 500 (only hits unusual files)
bn == 10000 -- t = 100 (only hits especially common lines)
bn == 31000 -- t = 31 (hack is at maximum power)
bn == 32000 -- t = 32 (hack could backfire, ease off)
