Make run_diff_index() use unpack_trees(), not read_tree()

A plain "git commit" would still run lstat() a lot more than necessary,
because wt_status_print() would cause the index to be repeatedly flushed
and re-read by wt_read_cache(), and that would cause the CE_UPTODATE bit
to be lost, resulting in the files in the index being lstat'ed three
times each.

The reason why wt-status.c ended up invalidating and re-reading the
cache multiple times was that it uses "run_diff_index()", which in turn
uses "read_tree()" to populate the index with *both* the old index and
the tree we want to compare against.

So this patch re-writes run_diff_index() to not use read_tree(), but
instead use "unpack_trees()" to diff the index to a tree.  That, in
turn, means that we don't need to modify the index itself, which then
means that we don't need to invalidate it and re-read it!

This, together with the lstat() optimizations, means that "git commit"
on the kernel tree really only needs to lstat() the index entries once.
That noticeably cuts down on the cached timings.

Best time before:

[torvalds@woody linux]$ time git commit > /dev/null
real    0m0.399s
user    0m0.232s
sys     0m0.164s

Best time after:

[torvalds@woody linux]$ time git commit > /dev/null
real    0m0.254s
user    0m0.140s
sys     0m0.112s

so it's a noticeable improvement in addition to being a nice conceptual
cleanup (it's really not that pretty that "run_diff_index()" dirties the
index!)

Doing an "strace -c" on it also shows that as it cuts the number of
lstat() calls by two thirds, it goes from being lstat()-limited to being
limited by getdents() (which is the readdir system call):

Before:
% time     seconds  usecs/call     calls    errors syscall
------ ----------- ----------- --------- --------- ----------------
60.69    0.000704           0     69230        31 lstat
23.62    0.000274           0      5522           getdents
8.36    0.000097           0      5508      2638 open
2.59    0.000030           0      2869           close
2.50    0.000029           0       274           write
1.47    0.000017           0      2844           fstat

After:
% time     seconds  usecs/call     calls    errors syscall
------ ----------- ----------- --------- --------- ----------------
45.17    0.000276           0      5522           getdents
26.51    0.000162           0     23112        31 lstat
19.80    0.000121           0      5503      2638 open
4.91    0.000030           0      2864           close
1.48    0.000020           0       274           write
1.34    0.000018           0      2844           fstat
...

It passes the test-suite for me, but this is another of one of those
really core functions, and certainly pretty subtle, so..

NOTE! The Linux lstat() system call is really quite cheap when everything
is cached, so the fact that this is quite noticeable on Linux is likely to
mean that it is *much* more noticeable on other operating systems. I bet
you'll see a much bigger performance improvement from this on Windows in
particular.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

diff --git a/diff-lib.c b/diff-lib.c
index 4a05b02cd0f53f61efa1d3acd43ca66870b0b33d..045f3cc58ec1e57e09b33bb2ade1bae9419ad450 100644 (file)
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -9,6 +9,7 @@
 #include "revision.h"
 #include "cache-tree.h"
 #include "path-list.h"
 #include "revision.h"
 #include "cache-tree.h"
 #include "path-list.h"

+#include "unpack-trees.h"

 
 /*
  * diff-files
 
 /*
  * diff-files


@@ -435,6 +436,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
                                continue;
                }
 
                                continue;
                }
 

+               if (ce_uptodate(ce))
+                       continue;

                if (lstat(ce->name, &st) < 0) {
                        if (errno != ENOENT && errno != ENOTDIR) {
                                perror(ce->name);
                if (lstat(ce->name, &st) < 0) {
                        if (errno != ENOENT && errno != ENOTDIR) {
                                perror(ce->name);


@@ -665,20 +668,133 @@ static void mark_merge_entries(void)
        }
 }
 
        }
 }
 

-int run_diff_index(struct rev_info *revs, int cached)
+/*
+ * This gets a mix of an existing index and a tree, one pathname entry
+ * at a time. The index entry may be a single stage-0 one, but it could
+ * also be multiple unmerged entries (in which case idx_pos/idx_nr will
+ * give you the position and number of entries in the index).
+ */
+static void do_oneway_diff(struct unpack_trees_options *o,
+       struct cache_entry *idx,
+       struct cache_entry *tree,
+       int idx_pos, int idx_nr)

 {
 {

-       int ret;
-       struct object *ent;
-       struct tree *tree;
-       const char *tree_name;
-       int match_missing = 0;
+       struct rev_info *revs = o->unpack_data;
+       int match_missing, cached;

 
        /*
         * Backward compatibility wart - "diff-index -m" does
 
        /*
         * Backward compatibility wart - "diff-index -m" does

-        * not mean "do not ignore merges", but totally different.
+        * not mean "do not ignore merges", but "match_missing".
+        *
+        * But with the revision flag parsing, that's found in
+        * "!revs->ignore_merges".
+        */
+       cached = o->index_only;
+       match_missing = !revs->ignore_merges;
+
+       if (cached && idx && ce_stage(idx)) {
+               if (tree)
+                       diff_unmerge(&revs->diffopt, idx->name, idx->ce_mode, idx->sha1);
+               return;
+       }
+
+       /*
+        * Something added to the tree?
+        */
+       if (!tree) {
+               show_new_file(revs, idx, cached, match_missing);
+               return;
+       }
+
+       /*
+        * Something removed from the tree?

         */
         */

-       if (!revs->ignore_merges)
-               match_missing = 1;
+       if (!idx) {
+               diff_index_show_file(revs, "-", tree, tree->sha1, tree->ce_mode);
+               return;
+       }
+
+       /* Show difference between old and new */
+       show_modified(revs, tree, idx, 1, cached, match_missing);
+}
+
+/*
+ * Count how many index entries go with the first one
+ */
+static inline int count_skip(const struct cache_entry *src, int pos)
+{
+       int skip = 1;
+
+       /* We can only have multiple entries if the first one is not stage-0 */
+       if (ce_stage(src)) {
+               struct cache_entry **p = active_cache + pos;
+               int namelen = ce_namelen(src);
+
+               for (;;) {
+                       const struct cache_entry *ce;
+                       pos++;
+                       if (pos >= active_nr)
+                               break;
+                       ce = *++p;
+                       if (ce_namelen(ce) != namelen)
+                               break;
+                       if (memcmp(ce->name, src->name, namelen))
+                               break;
+                       skip++;
+               }
+       }
+       return skip;
+}
+
+/*
+ * The unpack_trees() interface is designed for merging, so
+ * the different source entries are designed primarily for
+ * the source trees, with the old index being really mainly
+ * used for being replaced by the result.
+ *
+ * For diffing, the index is more important, and we only have a
+ * single tree.
+ *
+ * We're supposed to return how many index entries we want to skip.
+ *
+ * This wrapper makes it all more readable, and takes care of all
+ * the fairly complex unpack_trees() semantic requirements, including
+ * the skipping, the path matching, the type conflict cases etc.
+ */
+static int oneway_diff(struct cache_entry **src,
+       struct unpack_trees_options *o,
+       int index_pos)
+{
+       int skip = 0;
+       struct cache_entry *idx = src[0];
+       struct cache_entry *tree = src[1];
+       struct rev_info *revs = o->unpack_data;
+
+       if (index_pos >= 0)
+               skip = count_skip(idx, index_pos);
+
+       /*
+        * Unpack-trees generates a DF/conflict entry if
+        * there was a directory in the index and a tree
+        * in the tree. From a diff standpoint, that's a
+        * delete of the tree and a create of the file.
+        */
+       if (tree == o->df_conflict_entry)
+               tree = NULL;
+
+       if (ce_path_match(idx ? idx : tree, revs->prune_data))
+               do_oneway_diff(o, idx, tree, index_pos, skip);
+
+       return skip;
+}
+
+int run_diff_index(struct rev_info *revs, int cached)
+{
+       struct object *ent;
+       struct tree *tree;
+       const char *tree_name;
+       struct unpack_trees_options opts;
+       struct tree_desc t;

 
        mark_merge_entries();
 
 
        mark_merge_entries();
 


@@ -687,13 +803,20 @@ int run_diff_index(struct rev_info *revs, int cached)
        tree = parse_tree_indirect(ent->sha1);
        if (!tree)
                return error("bad tree object %s", tree_name);
        tree = parse_tree_indirect(ent->sha1);
        if (!tree)
                return error("bad tree object %s", tree_name);

-       if (read_tree(tree, 1, revs->prune_data))
-               return error("unable to read tree object %s", tree_name);
-       ret = diff_cache(revs, active_cache, active_nr, revs->prune_data,
-                        cached, match_missing);
+
+       memset(&opts, 0, sizeof(opts));
+       opts.head_idx = 1;
+       opts.index_only = cached;
+       opts.merge = 1;
+       opts.fn = oneway_diff;
+       opts.unpack_data = revs;
+
+       init_tree_desc(&t, tree->buffer, tree->size);
+       unpack_trees(1, &t, &opts);
+

        diffcore_std(&revs->diffopt);
        diff_flush(&revs->diffopt);
        diffcore_std(&revs->diffopt);
        diff_flush(&revs->diffopt);

-       return ret;
+       return 0;

 }
 
 int do_diff_cache(const unsigned char *tree_sha1, struct diff_options *opt)
 }
 
 int do_diff_cache(const unsigned char *tree_sha1, struct diff_options *opt)

diff --git a/unpack-trees.h b/unpack-trees.h
index 5517faafad6ca3f327c81ab6e8e5f2701ff0ad14..197a0044aa71fc9ca310b36b279b8382487a40a7 100644 (file)
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@ -25,6 +25,7 @@ struct unpack_trees_options {
        int merge_size;
 
        struct cache_entry *df_conflict_entry;
        int merge_size;
 
        struct cache_entry *df_conflict_entry;

+       void *unpack_data;

 };
 
 extern int unpack_trees(unsigned n, struct tree_desc *t,
 };
 
 extern int unpack_trees(unsigned n, struct tree_desc *t,

diff --git a/wt-status.c b/wt-status.c
index c0c247243b562a7579ff780b80dffef4774b60a9..27b946d55299d66031b31949da187ffb670e484b 100644 (file)
--- a/wt-status.c
+++ b/wt-status.c
@@ -217,19 +217,12 @@ static void wt_status_print_changed_cb(struct diff_queue_struct *q,
                wt_status_print_trailer(s);
 }
 
                wt_status_print_trailer(s);
 }
 

-static void wt_read_cache(struct wt_status *s)
-{
-       discard_cache();
-       read_cache_from(s->index_file);
-}
-

 static void wt_status_print_initial(struct wt_status *s)
 {
        int i;
        struct strbuf buf;
 
        strbuf_init(&buf, 0);
 static void wt_status_print_initial(struct wt_status *s)
 {
        int i;
        struct strbuf buf;
 
        strbuf_init(&buf, 0);

-       wt_read_cache(s);

        if (active_nr) {
                s->commitable = 1;
                wt_status_print_cached_header(s);
        if (active_nr) {
                s->commitable = 1;
                wt_status_print_cached_header(s);


@@ -256,7 +249,6 @@ static void wt_status_print_updated(struct wt_status *s)
        rev.diffopt.detect_rename = 1;
        rev.diffopt.rename_limit = 100;
        rev.diffopt.break_opt = 0;
        rev.diffopt.detect_rename = 1;
        rev.diffopt.rename_limit = 100;
        rev.diffopt.break_opt = 0;

-       wt_read_cache(s);

        run_diff_index(&rev, 1);
 }
 
        run_diff_index(&rev, 1);
 }
 


@@ -268,7 +260,6 @@ static void wt_status_print_changed(struct wt_status *s)
        rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
        rev.diffopt.format_callback = wt_status_print_changed_cb;
        rev.diffopt.format_callback_data = s;
        rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
        rev.diffopt.format_callback = wt_status_print_changed_cb;
        rev.diffopt.format_callback_data = s;

-       wt_read_cache(s);

        run_diff_files(&rev, 0);
 }
 
        run_diff_files(&rev, 0);
 }
 


@@ -335,7 +326,6 @@ static void wt_status_print_verbose(struct wt_status *s)
        setup_revisions(0, NULL, &rev, s->reference);
        rev.diffopt.output_format |= DIFF_FORMAT_PATCH;
        rev.diffopt.detect_rename = 1;
        setup_revisions(0, NULL, &rev, s->reference);
        rev.diffopt.output_format |= DIFF_FORMAT_PATCH;
        rev.diffopt.detect_rename = 1;

-       wt_read_cache(s);

        run_diff_index(&rev, 1);
 
        fflush(stdout);
        run_diff_index(&rev, 1);
 
        fflush(stdout);