Re: [PATCH v2] Simplify maildir_delayed_parsing().

--gkgqbwv34z7wg7zp
Content-Type: text/plain; protected-headers=v1; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
From: Alejandro Colomar <[email protected]>
To: [email protected]
Subject: Re: [PATCH v2] Simplify maildir_delayed_parsing().
Message-ID: <al4CFF6gzkWynxZ-@devuan>
References: <[email protected]>
MIME-Version: 1.0
In-Reply-To: <[email protected]>

Hi Kevin,

On 2026-07-20T12:53:54+0800, Kevin J. McCarthy wrote:
> Embed the DO_SORT() and the call to skip_duplicates() so I can
> understand it better.  Then remove the duplicate call to
> mutt_buffer_printf().
>=20
> It seems the sort is delayed until after any initial md entries
> without a header were first skipped, to make the sort faster.  There
> could be a very small number of "new" entries with a header, and if
> they are not at the beginning this could speed the sort up a lot.
>=20
> However putting the sort inside the loop with a DO_SORT() macro and a
> call to skip_duplicates() makes the code over-complicated and hard to
> understand.  Place the maildir_sort() call before the loop and add
> some comments.
>=20
> Remove the header_parsed bit, because it is only set inside this loop,
> and only checked inside this function.  Since the function is not
> called twice on the same md list, the header_parsed_bit field is not
> needed.
>=20
> Commit f2eef427 seems to indicate adding the sort was for reducing
> seek time.  I'm not a good judge of whether that's effective anymore,
> but I added a comment to at least indicate why the sort is being done.
> ---
>=20
> This seems to be working, so I'll just send it out now.
>=20
> The patch should look similar to the #3 and #4 from the last version.
>=20
> The only difference is that I added back the "skip initial md entries
> without md->h" before the sort.  I think in many cases, for maildir,
> there may be a small number of new entries.  It's a bit random where
> they are, but I'm still going to keep the optimization for now.
>=20
>  mh.c | 96 +++++++++++++++++++++---------------------------------------
>  1 file changed, 33 insertions(+), 63 deletions(-)

It's difficult to review the patch, but assuming it works, the
simplification seems nice.


Have a lovely day!
Alex

>=20
> diff --git a/mh.c b/mh.c
> index a74e94fb..ec9d07c6 100644
> --- a/mh.c
> +++ b/mh.c
> @@ -65,7 +65,6 @@ struct maildir
>  {
>    HEADER *h;
>    char *canon_fname;
> -  unsigned header_parsed:1;
>  #ifdef HAVE_DIRENT_D_INO
>    ino_t inode;
>  #endif /* HAVE_DIRENT_D_INO */
> @@ -1101,40 +1100,18 @@ static void mh_sort_natural(CONTEXT *ctx, struct =
maildir **md)
>    *md =3D maildir_sort(*md, (size_t) -1, md_cmp_path);
>  }
> =20
> -#if HAVE_DIRENT_D_INO
> -static struct maildir *skip_duplicates(struct maildir *p, struct maildir=
 **last)
> -{
> -  /*
> -   * Skip ahead to the next non-duplicate message.
> -   *
> -   * p should never reach NULL, because we couldn't have reached this po=
int unless
> -   * there was a message that needed to be parsed.
> -   *
> -   * the check for p->header_parsed is likely unnecessary since the dupe=
s will most
> -   * likely be at the head of the list.  but it is present for consisten=
cy with
> -   * the check at the top of the for() loop in maildir_delayed_parsing().
> -   */
> -  while (!p->h || p->header_parsed)
> -  {
> -    *last =3D p;
> -    p =3D p->next;
> -  }
> -  return p;
> -}
> -#endif
> -
>  /*
>   * This function does the second parsing pass
>   */
> -static void maildir_delayed_parsing(CONTEXT * ctx, struct maildir **md,
> +static void maildir_delayed_parsing(CONTEXT *ctx, struct maildir **md,
>                                      progress_t *progress)
>  {
> -  struct maildir *p, *last =3D NULL;
> +  struct maildir *p;
> +#if HAVE_DIRENT_D_INO
> +  struct maildir *last =3D NULL;
> +#endif
>    BUFFER *fn =3D NULL;
>    int count;
> -#if HAVE_DIRENT_D_INO
> -  int sort =3D 0;
> -#endif
>  #if USE_HCACHE
>    header_cache_t *hc =3D NULL;
>    void *data;
> @@ -1143,46 +1120,41 @@ static void maildir_delayed_parsing(CONTEXT * ctx=
, struct maildir **md,
>    int ret;
>  #endif
> =20
> -#if HAVE_DIRENT_D_INO
> -#define DO_SORT()                                                       \
> -  do                                                                    \
> -  {                                                                     \
> -    if (!sort)                                                          \
> -    {                                                                   \
> -      muttdbg(4, "maildir: need to sort %s by inode", ctx->path);       \
> -      p =3D maildir_sort(p, (size_t) -1, md_cmp_inode);                 =
  \
> -      if (!last)                                                        \
> -        *md =3D p;                                                      =
  \
> -      else                                                              \
> -        last->next =3D p;                                               =
  \
> -      sort =3D 1;                                                       =
  \
> -      p =3D skip_duplicates(p, &last);                                  =
  \
> -      mutt_buffer_printf(fn, "%s/%s", ctx->path, p->h->path);           \
> -    }                                                                   \
> -  } while (0)
> -#else
> -#define DO_SORT()       /* nothing */
> -#endif
> -
>  #if USE_HCACHE
>    hc =3D mutt_hcache_open(HeaderCache, ctx->path, NULL);
>  #endif
> -
>    fn =3D mutt_buffer_pool_get();
> +  p =3D *md;
> =20
> -  for (p =3D *md, count =3D 0; p; p =3D p->next, count++)
> +  /*
> +   * If available, sort by inode number to reduce seek time.
> +   */
> +#if HAVE_DIRENT_D_INO
> +  /* Skip over any initial entries without a header to make sorting fast=
er. */
> +  for (; p && !p->h; p =3D p->next)
> +    last =3D p;
> +  if (!p)
> +    goto cleanup;
> +
> +  muttdbg(4, "sorting %s by inode", ctx->path);
> +  p =3D maildir_sort(p, (size_t) -1, md_cmp_inode);
> +
> +  /* Reattach the initial entries without a header, if any, to the sorte=
d list.
> +   * This is needed so that md is properly freed by the caller. */
> +  if (last)
> +    last->next =3D p;
> +  else
> +    *md =3D p;
> +#endif
> +
> +  for (count =3D 0; p; p =3D p->next, count++)
>    {
> -    if (! (p && p->h && !p->header_parsed))
> -    {
> -      last =3D p;
> +    if (!p->h)
>        continue;
> -    }
> =20
>      if (!ctx->quiet && progress)
>        mutt_progress_update(progress, count, -1);
> =20
> -    DO_SORT();
> -
>      mutt_buffer_printf(fn, "%s/%s", ctx->path, p->h->path);
> =20
>  #if USE_HCACHE
> @@ -1215,7 +1187,6 @@ static void maildir_delayed_parsing(CONTEXT * ctx, =
struct maildir **md,
> =20
>        if (maildir_parse_message(ctx->magic, mutt_b2s(fn), p->h->old, p->=
h))
>        {
> -        p->header_parsed =3D 1;
>  #if USE_HCACHE
>          if (ctx->magic =3D=3D MUTT_MH)
>            mutt_hcache_store(hc, p->h->path, p->h, 0, strlen, MUTT_GENERA=
TE_UIDVALIDITY);
> @@ -1229,16 +1200,15 @@ static void maildir_delayed_parsing(CONTEXT * ctx=
, struct maildir **md,
>      }
>      mutt_hcache_free(&data);
>  #endif
> -    last =3D p;
>    }
> +
> +#if HAVE_DIRENT_D_INO
> +cleanup:
> +#endif
>  #if USE_HCACHE
>    mutt_hcache_close(hc);
>  #endif
> -
>    mutt_buffer_pool_release(&fn);
> -
> -#undef DO_SORT
> -
>    mh_sort_natural(ctx, md);
>  }
> =20
> --=20
> 2.55.0
>=20

--=20
<https://www.alejandro-colomar.es>

--gkgqbwv34z7wg7zp
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEES7Jt9u9GbmlWADAi64mZXMKQwqkFAmpeAkwACgkQ64mZXMKQ
wqmgrw/+Kgq7y8hqPRnzHJVR7SQx6Rlz6ZiltZ7tynz8wlReYQ21ioWZINq1A+pr
LJoKSlrhEvkKFFVNPLNOr1FiqQfhyn2d7w51ZOgyxW1gT7FBkHoOOivZfpR5hpFi
aDm5K/NmH1Yhzs2wq7vlMCh5VCQEnhKrReKOF8fexF7qC8x+BfSnSD7UtYFJjIHL
wAjb3vQGDNsJg4NLTrBSdiLt6X9srN1d5zsvLprPrFa+DsTmBAF+rK6IH09chN2Q
ciHfSwf99zi1GJd3rK7ofmNQTxOm+oY5Ew2WaAn06XfmAh9LeTfAn1OMlQCAJb1X
aZNJbryMkEvnE0itr23rdNb3rdZn7tc81FD/MwvZcdD3M0HTYHlP9JzYd8P6LsRy
8bxa1+4pdwsAGi4tbJjz1AxjS3fVx9Q36IKqWZLmZ3V+mBI3Uj7rVce3I9mGOgKM
Bkb3h+pLe77fE7OsFWT491A87eSzTP5r9xEZFgpFPiq6ivqHHdQhJn8zrsuH/eHt
cykzkMR/MoTzPYsOfHJ5Pr90/cA8fqfSuqq/tdosZl5fo6qb+YIPyJ/PdiQLXtW3
+0HIiOhdS6Bx8sWjigrs7urjpp74oTtZGyP8dv+2i1jauOVnkqcMBDi59oZK4W3h
VGoyEQkkwgaUoVfLUoN26k7VI40N6ZLSmmADVTWeGabgbr3zYnU=
=QjC1
-----END PGP SIGNATURE-----

--gkgqbwv34z7wg7zp--