Re: [PATCH] config: Introduce --patience config variable

Jeff King [off-list ref] writes:

On Tue, Mar 06, 2012 at 11:59:42AM +0100, Michal Privoznik wrote:

quoted

--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt

@@ -86,6 +86,9 @@ diff.mnemonicprefix::
 diff.noprefix::
 	If set, 'git diff' does not show any source or destination prefix.
 
+diff.patience:
+    If set, 'git diff' will use patience algorithm.
+

Should this be a boolean? Or should we actually have a diff.algorithm
option where you specify the algorithm you want (e.g., "diff.algorithm =
patience")? That would free us up later to more easily add new values.

In particular, I am thinking about --minimal. It is mutually exclusive
with --patience, and is simply ignored if you use patience diff.
we perhaps have "diff.algorithm" which can be one of "myers", "minimal"
(which is really myers + the minimal flag), and "patience".

Don't forget "histogram".  I have no idea why it's not documented
(evidently 8c912eea slipped through the review cracks) but --histogram
is supported since 1.7.7.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

8c912ee (teach --histogram to diff, 2011-07-12) claimed histogram diff
was faster than both Myers and patience.

We have since incorporated a performance testing framework, so add a
test that compares the various diff tasks performed in a real 'log -p'
workload.  This does indeed show that histogram diff slightly beats
Myers, while patience is much slower than the others.

Signed-off-by: Thomas Rast <redacted>
---

The 3000 is pretty arbitrary but makes for a nice test duration.

I'm reluctant to put numbers into the message, since the whole point
of the perf test framework is that you can easily get them too.  But
here's what I'm seeing:

  4000.1: log -3000 (baseline)          0.04(0.02+0.01)                                                     
  4000.2: log --raw -3000 (tree-only)   0.49(0.38+0.09)                                                     
  4000.3: log -p -3000 (Myers)          1.93(1.75+0.17)
  4000.4: log -p -3000 --histogram      1.90(1.74+0.15)
  4000.5: log -p -3000 --patience       2.25(2.07+0.16)

 t/perf/p4000-diff-algorithms.sh |   29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)
 create mode 100755 t/perf/p4000-diff-algorithms.sh

diff --git a/t/perf/p4000-diff-algorithms.sh b/t/perf/p4000-diff-algorithms.sh
new file mode 100755
index 0000000..d6e505c
--- /dev/null
+++ b/t/perf/p4000-diff-algorithms.sh

@@ -0,0 +1,29 @@
+#!/bin/sh
+
+test_description="Tests diff generation performance"
+
+. ./perf-lib.sh
+
+test_perf_default_repo
+
+test_perf 'log -3000 (baseline)' '
+	git log -1000 >/dev/null
+'
+
+test_perf 'log --raw -3000 (tree-only)' '
+	git log --raw -3000 >/dev/null
+'
+
+test_perf 'log -p -3000 (Myers)' '
+	git log -p -3000 >/dev/null
+'
+
+test_perf 'log -p -3000 --histogram' '
+	git log -p -3000 --histogram >/dev/null
+'
+
+test_perf 'log -p -3000 --patience' '
+	git log -p -3000 --patience >/dev/null
+'
+
+test_done

-- 
1.7.9.2.467.g7fee4

Signed-off-by: Thomas Rast <redacted>
---

This is only the minimal update.  I think in the long run, we should
add a note saying why we support all of them.  BUt off hand I didn't
have any substantial evidence in favour of patience that could be used
as an argument.

 Documentation/diff-options.txt |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 87f0a5f..7d4566f 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt

@@ -52,6 +52,9 @@ endif::git-format-patch[]
 --patience::
 	Generate a diff using the "patience diff" algorithm.
 
+--histogram::
+	Generate a diff using the "histogram diff" algorithm.
+
 --stat[=<width>[,<name-width>[,<count>]]]::
 	Generate a diffstat. By default, as much space as necessary
 	will be used for the filename part, and the rest for the graph

-- 
1.7.9.2.467.g7fee4

On Tue, Mar 06, 2012 at 02:01:42PM +0100, Thomas Rast wrote:

quoted

quoted

--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt

@@ -86,6 +86,9 @@ diff.mnemonicprefix::
 diff.noprefix::
 	If set, 'git diff' does not show any source or destination prefix.
 
+diff.patience:
+    If set, 'git diff' will use patience algorithm.
+

Should this be a boolean? Or should we actually have a diff.algorithm
option where you specify the algorithm you want (e.g., "diff.algorithm =
patience")? That would free us up later to more easily add new values.

In particular, I am thinking about --minimal. It is mutually exclusive
with --patience, and is simply ignored if you use patience diff.
we perhaps have "diff.algorithm" which can be one of "myers", "minimal"
(which is really myers + the minimal flag), and "patience".

Don't forget "histogram".  I have no idea why it's not documented
(evidently 8c912eea slipped through the review cracks) but --histogram
is supported since 1.7.7.

Ah, thanks. I had the vague feeling that we had a third algorithm
already, but I didn't see it in the docs. So yeah, I really think this
should be diff.algorithm, with a value of "myers", "patience", or
"histogram" (and possibly "minimal", depending how we want to treat
that).

-Peff

On 06.03.2012 14:01, Thomas Rast wrote:

Jeff King [off-list ref] writes:

quoted

On Tue, Mar 06, 2012 at 11:59:42AM +0100, Michal Privoznik wrote:

quoted

--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt

@@ -86,6 +86,9 @@ diff.mnemonicprefix::
 diff.noprefix::
 	If set, 'git diff' does not show any source or destination prefix.
 
+diff.patience:
+    If set, 'git diff' will use patience algorithm.
+

Should this be a boolean? Or should we actually have a diff.algorithm
option where you specify the algorithm you want (e.g., "diff.algorithm =
patience")? That would free us up later to more easily add new values.

In particular, I am thinking about --minimal. It is mutually exclusive
with --patience, and is simply ignored if you use patience diff.
we perhaps have "diff.algorithm" which can be one of "myers", "minimal"
(which is really myers + the minimal flag), and "patience".

Don't forget "histogram".  I have no idea why it's not documented
(evidently 8c912eea slipped through the review cracks) but --histogram
is supported since 1.7.7.

Okay guys. I'll got with diff.algorithm = [patience | minimal |
histogram | myers] then. What I am not sure about is how to threat case
when user have say algorithm = patience set in config but want to use
myers. I guess we need --myers option then, don't we?

Michal

Am 06.03.2012 14:15, schrieb Thomas Rast:

8c912ee (teach --histogram to diff, 2011-07-12) claimed histogram diff
was faster than both Myers and patience.

We have since incorporated a performance testing framework, so add a
test that compares the various diff tasks performed in a real 'log -p'
workload.  This does indeed show that histogram diff slightly beats
Myers, while patience is much slower than the others.

Signed-off-by: Thomas Rast<redacted>
---

The 3000 is pretty arbitrary but makes for a nice test duration.

I'm reluctant to put numbers into the message, since the whole point
of the perf test framework is that you can easily get them too.  But
here's what I'm seeing:

   4000.1: log -3000 (baseline)          0.04(0.02+0.01)
   4000.2: log --raw -3000 (tree-only)   0.49(0.38+0.09)
   4000.3: log -p -3000 (Myers)          1.93(1.75+0.17)
   4000.4: log -p -3000 --histogram      1.90(1.74+0.15)
   4000.5: log -p -3000 --patience       2.25(2.07+0.16)

Just a data point: --histogram is slightly slower for me:

   Test                                  this tree
   -----------------------------------------------------
   4000.1: log -3000 (baseline)          0.07(0.07+0.00)
   4000.2: log --raw -3000 (tree-only)   0.35(0.31+0.04)
   4000.3: log -p -3000 (Myers)          1.50(1.40+0.08)
   4000.4: log -p -3000 --histogram      1.54(1.48+0.05)
   4000.5: log -p -3000 --patience       1.79(1.71+0.06)

(baseline with -3000)

René