git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Emily Shaffer <emilyshaffer@google.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
	Derrick Stolee <derrickstolee@github.com>,
	Elijah Newren <newren@gmail.com>
Subject: Re: [PATCH 0/4] test-tool: split up "read-cache" tool
Date: Wed, 30 Jun 2021 15:02:41 -0700	[thread overview]
Message-ID: <YNzp4WIlZIzqD7PU@google.com> (raw)
In-Reply-To: <cover-0.4-0000000000-20210607T115454Z-avarab@gmail.com>

On Mon, Jun 07, 2021 at 01:58:23PM +0200, Ævar Arnfjörð Bjarmason wrote:
> 
> When the sparse index topic was being discussed I suggested that the
> t/helper/read-cache.c tool was getting to the point of doing too many
> things and should be split up.
> 
> Since that series has landed on master here's that suggestion again in
> the form of patches on top of master. The 4/4 patch is a "while I was
> at it" addition of an extra perf test for index refreshing.
> 
> 1. https://lore.kernel.org/git/20210317132814.30175-6-avarab@gmail.com/
> 

Since "What's cooking" mentioned this series was starved for review, we
took a look at it today. Most of my comments are pretty general, so I'll
reply primarily to the cover letter instead.

The result after this series is that we have three forms of 'test-tool
read-cache':
 - one for unit testing, with no iteration (test-tool read-cache)
 - one for not-so-perfy iteration (test-tool read-cache-again)
 - one for perfy iteration and refresh_index benching (test-tool
   read-cache-perf)

Before I read patch 4, I said, "'again' and 'perf' have a lot of code in
common, but I guess we are trying to reduce the amount of overhead for
tight-loop performance testing, so OK." But patch 4 adds an alternative
body for the inside of the loop, which *looks* not very performant
(although is probably optimized well) by checking an unchanging bool on
every iteration of the loop.

I tend to think it would be easier to read
and understand to have two forms of 'test-tool read-cache' - one which
iterates and one which does not. Maybe the one that iterates should be
called -perf, maybe it should be called something else, whatever. And
perhaps it makes sense for the iterating one to look like so (heavily
pseudocoded, hope you can follow along with the rough sketch):

  enum iteration_mode;

  parse_options();
  if (should_do_again) {
    iteration_mode = AGAIN;
  else if (should_do_perf) {
    iteration_mode = PERF;
  else if (should_do_refresh) {
    iteration_mode = REFRESH;
  }

  while (passes--)
    switch (iteration_mode) {
    case AGAIN:
      read index;
      refresh index;
      index_name_pos;
      error reporting;
      write_file;
      discard_index;
      break;
    case PERF:
      read index;
      discard index;
      break;
    case REFRESH:
      refresh_index;
      break;
    }

This would put all our "loop lots of times for performance benchmarking"
into one place. We know that the switch statement is very performant,
especially if we manage to const-ify iteration_mode.  The cases make it
very clear that the body of the loop is being swapped out depending on
the arguments, and that entirely different behavior is happening in each
scenario.

There's also an orthogonal bit of cleanup here by moving to
parse_options(), which I am excited about in general :) but which I
think wasn't done very cleanly in this series. In patch 1, the commit
message makes no mention of the fairly significant refactor happening to
move 'test-tool read-cache' to parse_options(), and I think the mix of
cut&paste with refactor makes the patch a little muddy. What about a
series like so:

 1/3: teach test-tool read-cache to use parse_options()
 2/3: add test-tool read-cache-(perf|iterating|whatever)
 3/3: teach test-tool read-cache-perf "--refresh"


Thanks, and hopefully this is a welcome necromancy and not an annoying
one ;)

 - Emily

  parent reply	other threads:[~2021-06-30 22:03 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-07 11:58 [PATCH 0/4] test-tool: split up "read-cache" tool Ævar Arnfjörð Bjarmason
2021-06-07 11:58 ` [PATCH 1/4] test-tool: split up test-tool read-cache Ævar Arnfjörð Bjarmason
2021-06-07 11:58 ` [PATCH 2/4] test-tools: migrate read-cache-perf to parse_options() Ævar Arnfjörð Bjarmason
2021-06-07 11:58 ` [PATCH 3/4] test-tools: migrate read-cache-again " Ævar Arnfjörð Bjarmason
2021-06-07 11:58 ` [PATCH 4/4] read-cache perf: add a perf test for refresh_index() Ævar Arnfjörð Bjarmason
2021-06-07 22:50 ` [PATCH 0/4] test-tool: split up "read-cache" tool Junio C Hamano
2021-06-08 11:14   ` Ævar Arnfjörð Bjarmason
2021-06-08 23:26     ` Junio C Hamano
2021-06-30 22:02 ` Emily Shaffer [this message]
2021-06-30 22:24   ` Ævar Arnfjörð Bjarmason
2021-08-24  9:15 ` [PATCH v2 " Ævar Arnfjörð Bjarmason
2021-08-24  9:15   ` [PATCH v2 1/4] test-tool: split up test-tool read-cache Ævar Arnfjörð Bjarmason
2021-08-24  9:15   ` [PATCH v2 2/4] test-tool: migrate read-cache-perf to parse_options() Ævar Arnfjörð Bjarmason
2021-08-24  9:15   ` [PATCH v2 3/4] test-tool: migrate read-cache-again " Ævar Arnfjörð Bjarmason
2021-08-24  9:15   ` [PATCH v2 4/4] read-cache perf: add a perf test for refresh_index() Ævar Arnfjörð Bjarmason
2021-08-24 22:19   ` [PATCH v2 0/4] test-tool: split up "read-cache" tool Taylor Blau
2021-08-25  0:18     ` Junio C Hamano
2021-08-27  7:24       ` Ævar Arnfjörð Bjarmason

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YNzp4WIlZIzqD7PU@google.com \
    --to=emilyshaffer@google.com \
    --cc=avarab@gmail.com \
    --cc=derrickstolee@github.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=newren@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).