@matheusbsbs / matheus-bagliano / commits / d1f2d7e8ca

Fork 0

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>

Linus Torvalds committed Jan 19, 2008 at 17:27 UTC d1f2d7e8ca65504722108e2db710788f66c34c6c

3 files changed +138 -24

diff-lib.c

+137 -14

@@ -9,6 +9,7 @@

9 #include "revision.h"

10 #include "cache-tree.h"

11 #include "path-list.h"

12 +#include "unpack-trees.h"

14 /*

15 * diff-files

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

436 continue;

437 }

438

439 + if (ce_uptodate(ce))

440 + continue;

441 if (lstat(ce->name, &st) < 0) {

442 if (errno != ENOENT && errno != ENOTDIR) {

443 perror(ce->name);

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

668 }

669 }

670

668 -int run_diff_index(struct rev_info *revs, int cached)

671 +/*

672 + * This gets a mix of an existing index and a tree, one pathname entry

673 + * at a time. The index entry may be a single stage-0 one, but it could

674 + * also be multiple unmerged entries (in which case idx_pos/idx_nr will

675 + * give you the position and number of entries in the index).

676 + */

677 +static void do_oneway_diff(struct unpack_trees_options *o,

678 + struct cache_entry *idx,

679 + struct cache_entry *tree,

680 + int idx_pos, int idx_nr)

681 {

670 - int ret;

671 - struct object *ent;

672 - struct tree *tree;

673 - const char *tree_name;

674 - int match_missing = 0;

682 + struct rev_info *revs = o->unpack_data;

683 + int match_missing, cached;

684

685 /*

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

678 - * not mean "do not ignore merges", but totally different.

687 + * not mean "do not ignore merges", but "match_missing".

688 + *

689 + * But with the revision flag parsing, that's found in

690 + * "!revs->ignore_merges".

691 + */

692 + cached = o->index_only;

693 + match_missing = !revs->ignore_merges;

694 +

695 + if (cached && idx && ce_stage(idx)) {

696 + if (tree)

697 + diff_unmerge(&revs->diffopt, idx->name, idx->ce_mode, idx->sha1);

698 + return;

699 + }

700 +

701 + /*

702 + * Something added to the tree?

703 + */

704 + if (!tree) {

705 + show_new_file(revs, idx, cached, match_missing);

706 + return;

707 + }

708 +

709 + /*

710 + * Something removed from the tree?

711 */

680 - if (!revs->ignore_merges)

681 - match_missing = 1;

712 + if (!idx) {

713 + diff_index_show_file(revs, "-", tree, tree->sha1, tree->ce_mode);

714 + return;

715 + }

716 +

717 + /* Show difference between old and new */

718 + show_modified(revs, tree, idx, 1, cached, match_missing);

719 +}

720 +

721 +/*

722 + * Count how many index entries go with the first one

723 + */

724 +static inline int count_skip(const struct cache_entry *src, int pos)

725 +{

726 + int skip = 1;

727 +

728 + /* We can only have multiple entries if the first one is not stage-0 */

729 + if (ce_stage(src)) {

730 + struct cache_entry **p = active_cache + pos;

731 + int namelen = ce_namelen(src);

732 +

733 + for (;;) {

734 + const struct cache_entry *ce;

735 + pos++;

736 + if (pos >= active_nr)

737 + break;

738 + ce = *++p;

739 + if (ce_namelen(ce) != namelen)

740 + break;

741 + if (memcmp(ce->name, src->name, namelen))

742 + break;

743 + skip++;

744 + }

745 + }

746 + return skip;

747 +}

748 +

749 +/*

750 + * The unpack_trees() interface is designed for merging, so

751 + * the different source entries are designed primarily for

752 + * the source trees, with the old index being really mainly

753 + * used for being replaced by the result.

754 + *

755 + * For diffing, the index is more important, and we only have a

756 + * single tree.

757 + *

758 + * We're supposed to return how many index entries we want to skip.

759 + *

760 + * This wrapper makes it all more readable, and takes care of all

761 + * the fairly complex unpack_trees() semantic requirements, including

762 + * the skipping, the path matching, the type conflict cases etc.

763 + */

764 +static int oneway_diff(struct cache_entry **src,

765 + struct unpack_trees_options *o,

766 + int index_pos)

767 +{

768 + int skip = 0;

769 + struct cache_entry *idx = src[0];

770 + struct cache_entry *tree = src[1];

771 + struct rev_info *revs = o->unpack_data;

772 +

773 + if (index_pos >= 0)

774 + skip = count_skip(idx, index_pos);

775 +

776 + /*

777 + * Unpack-trees generates a DF/conflict entry if

778 + * there was a directory in the index and a tree

779 + * in the tree. From a diff standpoint, that's a

780 + * delete of the tree and a create of the file.

781 + */

782 + if (tree == o->df_conflict_entry)

783 + tree = NULL;

784 +

785 + if (ce_path_match(idx ? idx : tree, revs->prune_data))

786 + do_oneway_diff(o, idx, tree, index_pos, skip);

787 +

788 + return skip;

789 +}

790 +

791 +int run_diff_index(struct rev_info *revs, int cached)

792 +{

793 + struct object *ent;

794 + struct tree *tree;

795 + const char *tree_name;

796 + struct unpack_trees_options opts;

797 + struct tree_desc t;

798

799 mark_merge_entries();

800

@@ -687,13 +803,20 @@ int run_diff_index(struct rev_info *revs, int cached)

803 tree = parse_tree_indirect(ent->sha1);

804 if (!tree)

805 return error("bad tree object %s", tree_name);

690 - if (read_tree(tree, 1, revs->prune_data))

691 - return error("unable to read tree object %s", tree_name);

692 - ret = diff_cache(revs, active_cache, active_nr, revs->prune_data,

693 - cached, match_missing);

806 +

807 + memset(&opts, 0, sizeof(opts));

808 + opts.head_idx = 1;

809 + opts.index_only = cached;

810 + opts.merge = 1;

811 + opts.fn = oneway_diff;

812 + opts.unpack_data = revs;

813 +

814 + init_tree_desc(&t, tree->buffer, tree->size);

815 + unpack_trees(1, &t, &opts);

816 +

817 diffcore_std(&revs->diffopt);

818 diff_flush(&revs->diffopt);

696 - return ret;

819 + return 0;

820 }

821

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

unpack-trees.h

@@ -25,6 +25,7 @@ struct unpack_trees_options {

25 int merge_size;

27 struct cache_entry *df_conflict_entry;

28 + void *unpack_data;

29 };

31 extern int unpack_trees(unsigned n, struct tree_desc *t,

wt-status.c

-10

@@ -217,19 +217,12 @@ static void wt_status_print_changed_cb(struct diff_queue_struct *q,

217 wt_status_print_trailer(s);

218 }

219

220 -static void wt_read_cache(struct wt_status *s)

221 -{

222 - discard_cache();

223 - read_cache_from(s->index_file);

224 -}

225 -

220 static void wt_status_print_initial(struct wt_status *s)

221 {

222 int i;

223 struct strbuf buf;

224

225 strbuf_init(&buf, 0);

232 - wt_read_cache(s);

226 if (active_nr) {

227 s->commitable = 1;

228 wt_status_print_cached_header(s);

@@ -256,7 +249,6 @@ static void wt_status_print_updated(struct wt_status *s)

249 rev.diffopt.detect_rename = 1;

250 rev.diffopt.rename_limit = 100;

251 rev.diffopt.break_opt = 0;

259 - wt_read_cache(s);

252 run_diff_index(&rev, 1);

253 }

254

@@ -268,7 +260,6 @@ static void wt_status_print_changed(struct wt_status *s)

260 rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;

261 rev.diffopt.format_callback = wt_status_print_changed_cb;

262 rev.diffopt.format_callback_data = s;

271 - wt_read_cache(s);

263 run_diff_files(&rev, 0);

264 }

265

@@ -335,7 +326,6 @@ static void wt_status_print_verbose(struct wt_status *s)

326 setup_revisions(0, NULL, &rev, s->reference);

327 rev.diffopt.output_format |= DIFF_FORMAT_PATCH;

328 rev.diffopt.detect_rename = 1;

338 - wt_read_cache(s);

329 run_diff_index(&rev, 1);

330

331 fflush(stdout);