Re: [PATCH 01/19] trace.c: add GIT_TRACE_PACK_STATS for pack usage statistics
On Thu, Mar 10, 2016 at 7:05 AM, David Turner wrote: > On Wed, 2016-03-09 at 14:58 -0800, Junio C Hamano wrote: >> David Turner writes: > ... >> > trace_stats() is intended for GIT_TRACE_*_STATS variable group and >> > GIT_TRACE_PACK_STATS is more like an example how new vars can be >> > added. > ... > >> > + pack_access_nr++; >> > } >> >> This looks rather half-hearted, in that those who are interested in >> this new number can run "wc -l" on the pack-access trace log without >> adding a new "stats" anyway. It may make the "stats" far more >> useful to keep track of the summary information of what would be >> written to the pack access log and add to the report_pack_stats() >> output things like the average and median distance of seeks >> (i.e. differences in the "obj_offset" into the same pack by two >> adjacent pack accesse) and the variance, for example? > > On reflection, I do not think I need this patch; it was in Duy's series > and the index stats patch would need to be rewritten slightly to get > rid of it, but I'm OK with that. I wanted to make minimal changes to > Duy's patches, but I'd rather make larger changes than do work on > patches I don't need. > > So unless Duy objects, I'm going to drop this from the series. I wanted something to verify from the tests and this was the best I could come up. But since I added trace_printf_key(&trace_exclude,... I think that could serve better to show what's going on, and we can easily summarize from the trace if we want to. That's one option. If you can adjust the tests another way, I'm fine too. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/19] trace.c: add GIT_TRACE_PACK_STATS for pack usage statistics
On Wed, 2016-03-09 at 14:58 -0800, Junio C Hamano wrote: > David Turner writes: ... > > trace_stats() is intended for GIT_TRACE_*_STATS variable group and > > GIT_TRACE_PACK_STATS is more like an example how new vars can be > > added. ... > > + pack_access_nr++; > > } > > This looks rather half-hearted, in that those who are interested in > this new number can run "wc -l" on the pack-access trace log without > adding a new "stats" anyway. It may make the "stats" far more > useful to keep track of the summary information of what would be > written to the pack access log and add to the report_pack_stats() > output things like the average and median distance of seeks > (i.e. differences in the "obj_offset" into the same pack by two > adjacent pack accesse) and the variance, for example? On reflection, I do not think I need this patch; it was in Duy's series and the index stats patch would need to be rewritten slightly to get rid of it, but I'm OK with that. I wanted to make minimal changes to Duy's patches, but I'd rather make larger changes than do work on patches I don't need. So unless Duy objects, I'm going to drop this from the series. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/19] trace.c: add GIT_TRACE_PACK_STATS for pack usage statistics
David Turner writes:
> From: Nguyễn Thái Ngọc Duy
>
> trace_stats() is intended for GIT_TRACE_*_STATS variable group and
> GIT_TRACE_PACK_STATS is more like an example how new vars can be added.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> Documentation/git.txt | 3 +++
> cache.h | 2 ++
> git.c | 1 +
> sha1_file.c | 24
> trace.c | 13 +
> trace.h | 1 +
> 6 files changed, 44 insertions(+)
>
> diff --git a/Documentation/git.txt b/Documentation/git.txt
> index 2754af8..794271e 100644
> --- a/Documentation/git.txt
> +++ b/Documentation/git.txt
> @@ -1055,6 +1055,9 @@ of clones and fetches.
> time of each Git command.
> See 'GIT_TRACE' for available trace output options.
>
> +'GIT_TRACE_PACK_STATS'::
> + Print various statistics.
"Various" is quite vague.
Perhaps a new description in Documentation/technical/ might be a
good thing to have?
> +void report_pack_stats(struct trace_key *key)
> +{
> + trace_printf_key(key, "\n"
> + "pack_report: getpagesize()= %10" SZ_FMT
> "\n"
> + "pack_report: core.packedGitWindowSize = %10" SZ_FMT
> "\n"
> + "pack_report: core.packedGitLimit = %10" SZ_FMT
> "\n"
> + "pack_report: pack_used_ctr= %10u\n"
> + "pack_report: pack_mmap_calls = %10u\n"
> + "pack_report: pack_open_windows= %10u / %10u\n"
> + "pack_report: pack_mapped = "
> + "%10" SZ_FMT " / %10" SZ_FMT "\n"
> + "pack_report: pack accesss = %10u\n",
> + sz_fmt(getpagesize()),
> + sz_fmt(packed_git_window_size),
> + sz_fmt(packed_git_limit),
> + pack_used_ctr,
> + pack_mmap_calls,
> + pack_open_windows, peak_pack_open_windows,
> + sz_fmt(pack_mapped), sz_fmt(peak_pack_mapped),
> + pack_access_nr);
> +}
> +
> /*
> * Open and mmap the index file at path, perform a couple of
> * consistency checks, then record its information to p. Return 0 on
> @@ -2238,6 +2261,7 @@ static void write_pack_access_log(struct packed_git *p,
> off_t obj_offset)
> static struct trace_key pack_access = TRACE_KEY_INIT(PACK_ACCESS);
> trace_printf_key(&pack_access, "%s %"PRIuMAX"\n",
>p->pack_name, (uintmax_t)obj_offset);
> + pack_access_nr++;
> }
This looks rather half-hearted, in that those who are interested in
this new number can run "wc -l" on the pack-access trace log without
adding a new "stats" anyway. It may make the "stats" far more
useful to keep track of the summary information of what would be
written to the pack access log and add to the report_pack_stats()
output things like the average and median distance of seeks
(i.e. differences in the "obj_offset" into the same pack by two
adjacent pack accesse) and the variance, for example?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
