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

Re: [PATCH v3 6/6] diff-lib: parallelize run_diff_files for submodules

From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date
Oct 21, 2022, 01:13 UTC
Message-ID
<221021.86pmemeyiu.gmgdl@evledraar.gmail.com>
In-Reply-To
<20221020232532.1128326-7-calvinwan@google.com>
On Thu, Oct 20 2022, Calvin Wan wrote:
Show 33 quoted lines
> @@ -103,6 +117,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
>  			      ? CE_MATCH_RACY_IS_DIRTY : 0);
>  	uint64_t start = getnanotime();
>  	struct index_state *istate = revs->diffopt.repo->index;
> +	struct string_list submodules = STRING_LIST_INIT_NODUP;
>  
>  	diff_set_mnemonic_prefix(&revs->diffopt, "i/", "w/");
>  
> @@ -227,6 +242,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
>  			newmode = ce->ce_mode;
>  		} else {
>  			struct stat st;
> +			unsigned ignore_untracked = 0;
> +			int defer_submodule_status = !!revs->repo;
>  
>  			changed = check_removed(istate, ce, &st);
>  			if (changed) {
> @@ -248,8 +265,24 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
>  			}
>  
>  			changed = match_stat_with_submodule(&revs->diffopt, ce, &st,
> -							    ce_option, &dirty_submodule);
> +							    ce_option, &dirty_submodule,
> +							    &defer_submodule_status,
> +							    &ignore_untracked);
>  			newmode = ce_mode_from_stat(ce, st.st_mode);
> +			if (defer_submodule_status) {
> +				struct submodule_status_util tmp = {
> +					.changed = changed,
> +					.dirty_submodule = 0,
> +					.ignore_untracked = ignore_untracked,
> +					.newmode = newmode,
> +					.ce = ce
Nit: We usually add the trailing "," to lists, except as a marker of
"don't add anything after this", i.e. only omit it when we have NULL as
the last element, but it's good to have it here.
> +				};
> +				struct string_list_item *item;
Nit: Another \n here to seperate the variables from the code, maybe...
> +				item = string_list_append(&submodules, ce->name);
> +				item->util = xmalloc(sizeof(tmp));
> +				memcpy(item->util, &tmp, sizeof(tmp));
This whole thing is much more readable, thanks.
Show 20 quoted lines
> +				continue;
> +			}
>  		}
>  
>  		if (!changed && !dirty_submodule) {
> @@ -268,6 +301,40 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
>  			    ce->name, 0, dirty_submodule);
>  
>  	}
> +	if (submodules.nr > 0) {
> +		int parallel_jobs;
> +		if (git_config_get_int("submodule.diffjobs", &parallel_jobs))
> +			parallel_jobs = 1;
> +		else if (!parallel_jobs)
> +			parallel_jobs = online_cpus();
> +		else if (parallel_jobs < 0)
> +			die(_("submodule.diffjobs cannot be negative"));
> +
> +		if (get_submodules_status(revs->repo, &submodules, parallel_jobs))
> +			BUG("submodule status failed");

A BUG() is probably too strong if we're invoking a subprocess, i.e. maybe the OS got tired of us and killed it, which is just an exception, not a bug in git.

And, I may be missing something, but later in that function we do:
	if (repo_read_index(r) < 0)
        	die(_("index file corrupt"));

Do we need to read the index there if we're just invoking a "status" command, isn't it reading it for us & reporting back?

Show 13 quoted lines
> +struct submodule_parallel_status {
> +	size_t index_count;
> +	int result;
> +
> +	struct string_list *submodule_names;
> +	struct repository *r;
> +
> +	/* Pending statuses by OIDs */
> +	struct status_task **oid_status_tasks;
> +	int oid_status_tasks_nr, oid_status_tasks_alloc;
> +};
> +
> +#define SPS_INIT { 0 }
Nit: It's good to add *_INIT macros sometimes, but here we just have a
private API that's only used once, which is...
Show 6 quoted lines
> [...]
> +int get_submodules_status(struct repository *r,
> +			  struct string_list *submodules,
> +			  int max_parallel_jobs)
> +{
> +	struct submodule_parallel_status sps = SPS_INIT;
...here, to copy some context around, and...
Show 20 quoted lines
> +	const struct run_process_parallel_opts opts = {
> +		.tr2_category = "submodule",
> +		.tr2_label = "parallel/status",
> +
> +		.processes = max_parallel_jobs,
> +		.hide_output = 1,
> +
> +		.get_next_task = get_next_submodule_status,
> +		.start_failure = status_start_failure,
> +		.pipe_output = status_pipe_output,
> +		.task_finished = status_finish,
> +		.data = &sps,
> +	};
> +
> +	sps.r = r;
> +
> +	if (repo_read_index(r) < 0)
> +		die(_("index file corrupt"));
> +
> +	sps.submodule_names = submodules;

...the net result is needing to first assign SPS_INIT, then add the "r" member, then "submodule_names". In this case this looks easier:

	struct submodule_parallel_status sps = {
		.r = r,
		.submodule_names = submodules,
	};
	const struct run_process_parallel_opts opts = {
        [...]
> +	string_list_sort(sps.submodule_names);
> +	run_processes_parallel(&opts);
Show 35 quoted lines
> +
>  struct submodule_parallel_fetch {
>  	/*
>  	 * The index of the last index entry processed by
> @@ -1445,6 +1459,13 @@ struct fetch_task {
>  	struct oid_array *commits; /* Ensure these commits are fetched */
>  };
>  
> +struct status_task {
> +	struct repository *repo;
> +	const char *path;
> +	unsigned dirty_submodule;
> +	int ignore_untracked;
> +};
> +
>  /**
>   * When a submodule is not defined in .gitmodules, we cannot access it
>   * via the regular submodule-config. Create a fake submodule, which we can
> @@ -1956,6 +1977,142 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
>  	return dirty_submodule;
>  }
>  
> +static struct status_task *
> +get_status_task_from_index(struct submodule_parallel_status *sps,
> +			   struct strbuf *err)
> +{
> +	for (; sps->index_count < sps->submodule_names->nr; sps->index_count++) {
> +		struct submodule_status_util *util = sps->submodule_names->items[sps->index_count].util;
> +		const struct cache_entry *ce = util->ce;
> +		struct status_task *task;
> +		struct strbuf buf = STRBUF_INIT;
> +		const char *git_dir;
> +
> +		strbuf_addf(&buf, "%s/.git", ce->name);
> +		git_dir = read_gitfile(buf.buf);

Okey, so the "NULL" variant of read_gitfile_gently(), as we don't care about the sort of error we got, but...

> +		if (!git_dir)
> +			git_dir = buf.buf;
> +		if (!is_git_directory(git_dir)) {

Isn't this something we could have asked read_gitfile_gently() about instead, i.e. the READ_GITFILE_ERR_NOT_A_REPO condition?

> +			if (is_directory(git_dir))
> +				die(_("'%s' not recognized as a git repository"), git_dir);

It would be a detour from this, but perhaps setup.c can be tasked with knowing about this edge case and returning an error code, but even if we punt on that we can just do the is_directory() here, but get the !is_git_directory() for free it seems.

Show 9 quoted lines
> +			strbuf_release(&buf);
> +			/* The submodule is not checked out, so it is not modified */
> +			util->dirty_submodule = 0;
> +			continue;
> +		}
> +		strbuf_release(&buf);
> +
> +		task = xmalloc(sizeof(*task));
> +		memset(task, 0, sizeof(*task));
s/xmalloc/xcalloc/?
> +		task->path = ce->name;
> +		task->dirty_submodule = util->dirty_submodule;
> +		task->ignore_untracked = util->ignore_untracked;

Cn do we the same readability trick here that we did with "struct submodule_status_util tmp = {" earlier & the memcpy()? Looks like it...

> +static int get_next_submodule_status(struct child_process *cp,
> +				     struct strbuf *err,
> +				     void *data, void **task_cb)
Nit: Too early wrapping, "void *data" should be at the previous line
(which won't exceed 79 chars)?
Show 7 quoted lines
> +{
> +	struct submodule_parallel_status *sps = data;
> +	struct status_task *task = get_status_task_from_index(sps, err);
> +
> +	if (!task) {
> +		return 0;
> +	}
Nit: Don't need {}-braces here.
Show 8 quoted lines
> +
> +	child_process_init(cp);
> +	prepare_submodule_repo_env_in_gitdir(&cp->env);
> +
> +	strvec_init(&cp->args);
> +	strvec_pushl(&cp->args, "status", "--porcelain=2", NULL);
> +	if (task->ignore_untracked)
> +		strvec_push(&cp->args, "-uno");
Nit: Maybe worth spelling out --untracked-files=no (or maybe "-uno" is
more idiomatic at this point...)
Show 29 quoted lines
> +
> +	prepare_submodule_repo_env(&cp->env);
> +	cp->git_cmd = 1;
> +	cp->no_stdin = 1;
> +	cp->dir = task->path;
> +	*task_cb = task;
> +	return 1;
> +}
> +
> +static int status_start_failure(struct strbuf *err,
> +			        void *cb, void *task_cb)
> +{
> +	struct submodule_parallel_status *sps = cb;
> +
> +	sps->result = 1;
> +	return 0;
> +}
> +
> +static void status_pipe_output(struct strbuf *process_out,
> +			       void *cb, void *task_cb)
> +{
> +	struct status_task *task = task_cb;
> +	struct string_list list = STRING_LIST_INIT_DUP;
> +
> +	string_list_split(&list, process_out->buf, '\n', -1);
> +
> +	for (size_t i = 0; i < list.nr; i++) {
> +		if (parse_status_porcelain(list.items[i].string,
> +				   strlen(list.items[i].string),
Nit: I think you can use for_each_string_list_item() here, saving some
verbosity..
> +int get_submodules_status(struct repository *r,
> +			  struct string_list *submodules,
> +			  int max_parallel_jobs);

s/int/size_t/ because at this point you've already die()'d in the one caller if it's <0 from the config parser, so we know it's unsigned (actually >1, but unsigned will have to do :).

Previous: Calvin WanNext: Calvin Wan
Message 18 of 19 in “submodule: parallelize diff”
  1. 0/6 submodule: parallelize diffCalvin Wan, Oct 20, 2022
  2. 1/6 run-command: add pipe_output_fn to run_processes_parallel_optsCalvin Wan, Oct 20, 2022
  3. Ævar Arnfjörð BjarmasonOct 21, 2022
  4. Calvin WanOct 24, 2022
  5. Junio C HamanoOct 21, 2022
  6. Calvin WanOct 24, 2022
  7. Junio C HamanoOct 24, 2022
  8. Calvin WanOct 25, 2022
  9. 2/6 run-command: add hide_output to run_processes_parallel_optsCalvin Wan, Oct 20, 2022
  10. Ævar Arnfjörð BjarmasonOct 21, 2022
  11. Calvin WanOct 24, 2022
  12. Ævar Arnfjörð BjarmasonOct 25, 2022
  13. Calvin WanOct 25, 2022
  14. 3/6 submodule: strbuf variable renameCalvin Wan, Oct 20, 2022
  15. 4/6 submodule: move status parsing into functionCalvin Wan, Oct 20, 2022
  16. 5/6 diff-lib: refactor match_stat_with_submoduleCalvin Wan, Oct 20, 2022
  17. 6/6 diff-lib: parallelize run_diff_files for submodulesCalvin Wan, Oct 20, 2022
  18. Ævar Arnfjörð BjarmasonOct 21, 2022
  19. Calvin WanNov 3, 2022

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.