Re: [PATCH v3 1/8] [RFC] dir: convert trace calls to trace2 equivalents
- From
Junio C Hamano <gitster@pobox.com>
- Date
- May 10, 2021, 04:49 UTC
- Message-ID
- <xmqqk0o7ry5c.fsf@gitster.g>
- In-Reply-To
- <9f1c0d78d739cc1da8517eea92e81a7bd162578f.1620503945.git.gitgitgadget@gmail.com>
"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 17 quoted lines
> +static void trace2_read_directory_statistics(struct dir_struct *dir,
> + struct repository *repo)
> +{
> + if (!dir->untracked)
> + return;
> + trace2_data_intmax("read_directory", repo,
> + "node-creation", dir->untracked->dir_created);
> + trace2_data_intmax("read_directory", repo,
> + "gitignore-invalidation",
> + dir->untracked->gitignore_invalidated);
> + trace2_data_intmax("read_directory", repo,
> + "directory-invalidation",
> + dir->untracked->dir_invalidated);
> + trace2_data_intmax("read_directory", repo,
> + "opendir", dir->untracked->dir_opened);
> +}
> +This obviously looks like an equivalent to what happens in the original inside the "if (dir->untracked)" block.
And we have a performance_{enter,leave} pair replaced with a region_[enter,leave} pair.
Show 5 quoted lines
> - trace_performance_enter();
> + trace2_region_enter("dir", "read_directory", istate->repo);
> ...
> - trace_performance_leave("read directory %.*s", len, path);
> + trace2_region_leave("dir", "read_directory", istate->repo);Show 19 quoted lines
> if (dir->untracked) {
> static int force_untracked_cache = -1;
> - static struct trace_key trace_untracked_stats = TRACE_KEY_INIT(UNTRACKED_STATS);
>
> if (force_untracked_cache < 0)
> force_untracked_cache =
> git_env_bool("GIT_FORCE_UNTRACKED_CACHE", 0);
> - trace_printf_key(&trace_untracked_stats,
> - "node creation: %u\n"
> - "gitignore invalidation: %u\n"
> - "directory invalidation: %u\n"
> - "opendir: %u\n",
> - dir->untracked->dir_created,
> - dir->untracked->gitignore_invalidated,
> - dir->untracked->dir_invalidated,
> - dir->untracked->dir_opened);
> if (force_untracked_cache &&
> dir->untracked == istate->untracked &&
> (dir->untracked->dir_opened ||Removal of the trace_printf() in the middle made the body of this if() statement much less distracting, which is good.
Show 7 quoted lines
> @@ -2811,6 +2818,9 @@ int read_directory(struct dir_struct *dir, struct index_state *istate, > FREE_AND_NULL(dir->untracked); > } > } > + > + if (trace2_is_enabled()) > + trace2_read_directory_statistics(dir, istate->repo);
This slightly changes the semantics in that the original did an equivalent emitting from inside the "if (dir->untracked)" block, but this call is hoisted outside, and the new helper knows how to be silent when untracked thing is not in effect, so the net effect at this step is the same. And if we ever add tracing statics that is relevant when !dir->untracked is true, the new code organization is easier to work with.
The only curious thing is the guard "if (trace2_is_enabled())"; correctness-wise, are there bad things going to happen if it is not here, or is this a performance hack, or is it more for its documentation value (meaning, it would be a bug if we later added things that are irrelevant when trace is not enabled to the helper)?
Show 5 quoted lines
> @@ -57,6 +57,19 @@ iuc () {
> return $ret
> }
>
> +get_relevant_traces() {Style. SP on both sides of "()".
Show 9 quoted lines
> + # From the GIT_TRACE2_PERF data of the form > + # $TIME $FILE:$LINE | d0 | main | data | r1 | ? | ? | read_directo | $RELEVANT_STAT > + # extract the $RELEVANT_STAT fields. We don't care about region_enter > + # or region_leave, or stats for things outside read_directory. > + INPUT_FILE=$1 > + OUTPUT_FILE=$2 > + grep data.*read_directo $INPUT_FILE \ > + | cut -d "|" -f 9 \ > + >$OUTPUT_FILE
Style. Wrapping the line after pipe '|' will allow you to omit the backslash. Also quote the redirection target, i.e. >"$OUTPUT_FILE", to help certain vintage of bash.
Those who are more familiar with the trace2 infrastructure may want to further comment, but it looked obvious and straightforward to me.
Thanks.