Fix built-in rebase perf regression
Get series with b4
b4 am [email protected]Versions newest first · linked by author and title
- v2 Latest3 patches 1 replyIn review
- v12 patches 10 repliesSuperseded
Patches in v2 No applies recorded yet
| # | Subject and files | Size | Replies | Applied |
|---|---|---|---|---|
| 1/3 | rebase: consolidate clean-up code before leaving reset_head()builtin/ | +18 | 0 replies | |
| 2/3 | rebase: prepare reset_head() for more flagsbuiltin/ | +6 | 0 replies | |
| 3/3 | built-in rebase: reinstate `checkout -q` behavior where appropriatebuiltin/ | +25 | 0 replies |
Reviews trailers per patch
No Reviewed-by, Acked-by or Tested-by trailers for v2 yet.
Cover letter [PATCH v2 0/3] Fix built-in rebase perf regression
In our tests with large repositories, we noticed a serious regression of the performance of git rebase when using the built-in vs the shell script version. It boils down to an incorrect conversion of a git checkout -q:
instead of using a twoway_merge as git checkout does, we used a oneway_merge as git reset does. The latter, however, calls lstat() on all files listed in the index, while the former essentially looks only at the files that are different between the given two revisions.
Let's reinstate the original behavior by introducing a flag to the reset_head() function to indicate whether we want to emulate reset --hard (in which case we use the oneway_merge, otherwise we use twoway_merge).
Johannes Schindelin (3):
rebase: consolidate clean-up code before leaving reset_head()
rebase: prepare reset_head() for more flags
built-in rebase: reinstate checkout -q behavior where appropriate
builtin/rebase.c | 79 ++++++++++++++++++++++++++++-------------------- 1 file changed, 46 insertions(+), 33 deletions(-)
base-commit: 8858448bb49332d353febc078ce4a3abcc962efe
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-72%2Fdscho%2Fbuiltin-rebase-perf-regression-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-72/dscho/builtin-rebase-perf-regression-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/72
Range-diff vs v1:
1: 64597fe827 ! 1: 28e24d98ab rebase: consolidate clean-up code before leaving reset_head()
@@ -11,6 +11,33 @@
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@
+ if (switch_to_branch && !starts_with(switch_to_branch, "refs/"))
+ BUG("Not a fully qualified branch: '%s'", switch_to_branch);
+
+- if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0)
+- return -1;
++ if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0) {
++ ret = -1;
++ goto leave_reset_head;
++ }
+
+ if (!oid) {
+ if (get_oid("HEAD", &head_oid)) {
+- rollback_lock_file(&lock);
+- return error(_("could not determine HEAD revision"));
++ ret = error(_("could not determine HEAD revision"));
++ goto leave_reset_head;
+ }
+ oid = &head_oid;
+ }
+@@
+ unpack_tree_opts.reset = 1;
+
+ if (read_index_unmerged(the_repository->index) < 0) {
+- rollback_lock_file(&lock);
+- return error(_("could not read index"));
++ ret = error(_("could not read index"));
++ goto leave_reset_head;
}
if (!fill_tree_descriptor(&desc, oid)) {
@@ -31,15 +58,17 @@
}
tree = parse_tree_indirect(oid);
-@@
+ prime_cache_tree(the_repository->index, tree);
- if (write_locked_index(the_repository->index, &lock, COMMIT_LOCK) < 0)
+- if (write_locked_index(the_repository->index, &lock, COMMIT_LOCK) < 0)
++ if (write_locked_index(the_repository->index, &lock, COMMIT_LOCK) < 0) {
ret = error(_("could not write index"));
- free((void *)desc.buffer);
-
- if (ret)
+-
+- if (ret)
- return ret;
+ goto leave_reset_head;
++ }
reflog_action = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);
strbuf_addf(&msg, "%s: ", reflog_action ? reflog_action : "rebase");
-: ---------- > 2: db963b2094 rebase: prepare reset_head() for more flags
2: 070092b430 ! 3: a7360b856f built-in rebase: reinstate `checkout -q` behavior where appropriate
@@ -20,15 +20,18 @@
@@
#define GIT_REFLOG_ACTION_ENVIRONMENT "GIT_REFLOG_ACTION"
+ #define RESET_HEAD_DETACH (1<<0)
++#define RESET_HEAD_HARD (1<<1)
+
static int reset_head(struct object_id *oid, const char *action,
-- const char *switch_to_branch, int detach_head,
-+ const char *switch_to_branch,
-+ int detach_head, int reset_hard,
+ const char *switch_to_branch, unsigned flags,
const char *reflog_orig_head, const char *reflog_head)
{
+ unsigned detach_head = flags & RESET_HEAD_DETACH;
++ unsigned reset_hard = flags & RESET_HEAD_HARD;
struct object_id head_oid;
- struct tree_desc desc;
-+ struct tree_desc desc[2];
++ struct tree_desc desc[2] = { { NULL }, { NULL } };
struct lock_file lock = LOCK_INIT;
struct unpack_trees_options unpack_tree_opts;
struct tree *tree;
@@ -42,18 +45,18 @@
if (switch_to_branch && !starts_with(switch_to_branch, "refs/"))
BUG("Not a fully qualified branch: '%s'", switch_to_branch);
@@
- if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0)
- return -1;
+ goto leave_reset_head;
+ }
- if (!oid) {
- if (get_oid("HEAD", &head_oid)) {
-- rollback_lock_file(&lock);
-- return error(_("could not determine HEAD revision"));
+- ret = error(_("could not determine HEAD revision"));
+- goto leave_reset_head;
- }
- oid = &head_oid;
-+ if (get_oid("HEAD", &head_oid)) {
-+ rollback_lock_file(&lock);
-+ return error(_("could not determine HEAD revision"));
++ if ((!oid || !reset_hard) && get_oid("HEAD", &head_oid)) {
++ ret = error(_("could not determine HEAD revision"));
++ goto leave_reset_head;
}
+ if (!oid)
@@ -70,7 +73,7 @@
unpack_tree_opts.merge = 1;
if (!detach_head)
@@
- return error(_("could not read index"));
+ goto leave_reset_head;
}
- if (!fill_tree_descriptor(&desc, oid)) {
@@ -104,7 +107,8 @@
string_list_clear(&merge_rr, 1);
- if (reset_head(NULL, "reset", NULL, 0, NULL, NULL) < 0)
-+ if (reset_head(NULL, "reset", NULL, 0, 1, NULL, NULL) < 0)
++ if (reset_head(NULL, "reset", NULL, RESET_HEAD_HARD,
++ NULL, NULL) < 0)
die(_("could not discard worktree changes"));
if (read_basic_state(&options))
exit(1);
@@ -113,7 +117,8 @@
exit(1);
if (reset_head(&options.orig_head, "reset",
- options.head_name, 0, NULL, NULL) < 0)
-+ options.head_name, 0, 1, NULL, NULL) < 0)
++ options.head_name, RESET_HEAD_HARD,
++ NULL, NULL) < 0)
die(_("could not move back to %s"),
oid_to_hex(&options.orig_head));
ret = finish_rebase(&options);
@@ -122,34 +127,7 @@
printf(_("Created autostash: %s\n"), buf.buf);
if (reset_head(&head->object.oid, "reset --hard",
- NULL, 0, NULL, NULL) < 0)
-+ NULL, 0, 1, NULL, NULL) < 0)
++ NULL, RESET_HEAD_HARD, NULL, NULL) < 0)
die(_("could not reset --hard"));
printf(_("HEAD is now at %s"),
find_unique_abbrev(&head->object.oid,
-@@
- strbuf_addf(&buf, "rebase: checkout %s",
- options.switch_to);
- if (reset_head(&oid, "checkout",
-- options.head_name, 0,
-+ options.head_name, 0, 0,
- NULL, NULL) < 0) {
- ret = !!error(_("could not switch to "
- "%s"),
-@@
- "it...\n"));
-
- strbuf_addf(&msg, "rebase: checkout %s", options.onto_name);
-- if (reset_head(&options.onto->object.oid, "checkout", NULL, 1,
-+ if (reset_head(&options.onto->object.oid, "checkout", NULL, 1, 0,
- NULL, msg.buf))
- die(_("Could not detach HEAD"));
- strbuf_release(&msg);
-@@
- strbuf_addf(&msg, "rebase finished: %s onto %s",
- options.head_name ? options.head_name : "detached HEAD",
- oid_to_hex(&options.onto->object.oid));
-- reset_head(NULL, "Fast-forwarded", options.head_name, 0,
-+ reset_head(NULL, "Fast-forwarded", options.head_name, 0, 0,
- "HEAD", msg.buf);
- strbuf_release(&msg);
- ret = !!finish_rebase(&options);Signature · 1 line
gitgitgadget