Re: [PATCH] revision: trace topo-walk statistics

Derrick Stolee [off-list ref] writes:

On 1/6/2021 8:37 PM, Junio C Hamano wrote:

quoted

"Derrick Stolee via GitGitGadget" [off-list ref] writes:

quoted

diff --git a/revision.c b/revision.c
index 9dff845bed6..1bb590ece78 100644
--- a/revision.c
+++ b/revision.c

@@ -3308,6 +3308,26 @@ struct topo_walk_info {
 	struct author_date_slab author_date;
 };
 
+static int topo_walk_atexit_registered;
+static unsigned int count_explore_walked;
+static unsigned int count_indegree_walked;
+static unsigned int count_topo_walked;

The revision walk machinery is designed to be callable more than
once during the lifetime of a process.  These make readers wonder
if they should be defined in "struct rev_info" to allow stats
collected per traversal.

That's possible, but the use of an at-exit method means we only
report one set of statistics and the 'struct rev_info' might
be defunct.

Ah, sorry for the noise.  Even after making multiple traversal we
want to report the aggregate.

It does limit how useful the statistics can be when there are
multiple 'struct rev_info's in use, but we also cannot trust
that the rev_infos are being cleaned up properly at the end
of the process to trigger the stats logging.

Of course, maybe that _is_ something we could guarantee, or
rather _should_ guarantee by patching any leaks. Seems like
a lot of work when these aggregate statistics will be
effective enough. But maybe I'm judging the potential work
too harshly?

But different subsystems would have different "per-invocation"
structure (e.g. "diff" uses "struct diff_options") and some may not
even have an appropriate structure to hang these stats on.  We may
want to design a more general mechanism that can be used to annotate
the subsystems uniformly.  While that could be a worthy longer term
goal, it certainly does not have to be part of this single-patch
topic, I would think.

On 1/6/2021 9:29 PM, Junio C Hamano wrote:

But different subsystems would have different "per-invocation"
structure (e.g. "diff" uses "struct diff_options") and some may not
even have an appropriate structure to hang these stats on.  We may
want to design a more general mechanism that can be used to annotate
the subsystems uniformly.  While that could be a worthy longer term
goal, it certainly does not have to be part of this single-patch
topic, I would think.

 
I will give this further thought.

Thanks,
-Stolee