git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v2 0/3] Fix built-in rebase perf regression

From
Johannes Schindelin <johannes.schindelin@gmx.de>
Date
Nov 12, 2018, 14:26 UTC
Message-ID
<nycvar.QRO.7.76.6.1811121521300.39@tvgsbejvaqbjf.bet>
In-Reply-To
<pull.72.v2.git.gitgitgadget@gmail.com>
Hi,
On Mon, 12 Nov 2018, Johannes Schindelin via GitGitGadget wrote:
Show 19 quoted lines
> 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(-)
I forgot to specify the changes vs v1:
- More error paths are not consolidated via `goto leave_reset_head`.
- The `desc` array is not initialized to all-zero, to avoid bogus
  addresses being passed to `free()`.
- The `detach_head` and `reset_hard` parameters have been consolidated
  into a `flags` parameter.
- The `reset_head()` function once again only initializes `head_oid`
  when needed.

Sorry for the omission, Johannes

Show 186 quoted lines
> 
> 
> 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);
> 
> -- 
> gitgitgadget
>
Previous: Johannes Schindelin via GitGitGadget
Message 18 of 18 in “Fix built-in rebase perf regression”
  1. 0/2 Fix built-in rebase perf regressionJohannes Schindelin via GitGitGadget, Nov 9, 2018
  2. 1/2 rebase: consolidate clean-up code before leaving reset_head()Johannes Schindelin via GitGitGadget, Nov 9, 2018
  3. Jeff KingNov 9, 2018
  4. Johannes SchindelinNov 9, 2018
  5. 2/2 built-in rebase: reinstate `checkout -q` behavior where appropriateJohannes Schindelin via GitGitGadget, Nov 9, 2018
  6. Jeff KingNov 9, 2018
  7. Johannes SchindelinNov 9, 2018
  8. Jeff KingNov 9, 2018
  9. Johannes SchindelinNov 12, 2018
  10. Junio C HamanoNov 12, 2018
  11. Johannes SchindelinNov 12, 2018
  12. Junio C HamanoNov 13, 2018
  13. Junio C HamanoNov 13, 2018
  14. 0/3 Fix built-in rebase perf regressionJohannes Schindelin via GitGitGadget, Nov 12, 2018
  15. 1/3 rebase: consolidate clean-up code before leaving reset_head()Johannes Schindelin via GitGitGadget, Nov 12, 2018
  16. 2/3 rebase: prepare reset_head() for more flagsJohannes Schindelin via GitGitGadget, Nov 12, 2018
  17. 3/3 built-in rebase: reinstate `checkout -q` behavior where appropriateJohannes Schindelin via GitGitGadget, Nov 12, 2018
  18. Johannes SchindelinNov 12, 2018

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.