Git development
 help / color / mirror / Atom feed
From: Patrick Steinhardt <ps@pks.im>
To: Tamir Duberstein <tamird@gmail.com>
Cc: git@vger.kernel.org, Paul Tarjan <github@paulisageek.com>,
	Junio C Hamano <gitster@pobox.com>, Jeff King <peff@peff.net>,
	Taylor Blau <me@ttaylorr.com>,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	Koji Nakamaru <koji.nakamaru@gree.net>
Subject: Re: [PATCH] fsmonitor: flush pending FSEvents before cookie wait
Date: Thu, 13 Aug 2026 11:03:11 +0200	[thread overview]
Message-ID: <an2IT78KDS94JqUt@pks.im> (raw)
In-Reply-To: <CAJ-ks9kQR77vH-56eS9tT-iXEnih+Z7SPRMs1gD_wTyg_6gZ_w@mail.gmail.com>

On Wed, Aug 12, 2026 at 09:19:08PM -0400, Tamir Duberstein wrote:
> On Tue, Aug 11, 2026 at 12:45 PM Tamir Duberstein <tamird@gmail.com> wrote:
[snip]
> > Got it. I agree with you that the flush call feels unnecessary under
> > the interpretation that passing 1ms to FSEventStreamCreate is the
> > equivalent of asking it to flush every 1ms. Empirically, though,
> > that's not the case, as described in the commit message.
> >
> > There's more precedent for this technique (found by agent, sorry):
> > watchman fixed a similar issue here:
> > https://github.com/facebook/watchman/commit/d1795de4ecab33672a89802318fe6f0122462194
> > and the documented it here:
> > https://github.com/facebook/watchman/commit/2f80886991ce81585ac0679c2b019fa0e4d9e9dd
> >
> > I agree this is unsatisfying.
> >
> > Does that help?

Those links definitely help to provide some more context, thanks!

> I did a bunch more digging and I'm withdrawing this patch. I haven't
> succeeded in proving that this fixes the performance issue. I'll
> resend in case this changes.

One major difference I notice there is that your patch uses
`FsEventStreamFlushAsync()`, whereas Watchman uses the `Sync()` variant.
That could help explain why it works for their use case, as the can now
guarantee that the cookie was indeed processed once that call finishes.
But with our `Async()` variant that's a guarantee that we cannot uphold,
and consequently we're essentially still racing with the timout.

Now we could of course try to use the synchronous variant ourselves. But
I'm a bit concerned that this may create new problems that we don't
really understand yet. Quite unfortunate indeed :/

Thanks!

Patrick

  reply	other threads:[~2026-08-13  9:03 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-07-21 21:04 [PATCH] fsmonitor: flush pending FSEvents before cookie wait Tamir Duberstein
2026-07-24  2:41 ` Koji Nakamaru
2026-07-24 20:38   ` Junio C Hamano
2026-08-04 22:13   ` Junio C Hamano
2026-08-05  7:59 ` Patrick Steinhardt
2026-08-11 15:22   ` Tamir Duberstein
2026-08-11 16:23     ` Patrick Steinhardt
2026-08-11 16:45       ` Tamir Duberstein
2026-08-13  1:19         ` Tamir Duberstein
2026-08-13  9:03           ` Patrick Steinhardt [this message]
2026-08-11 17:35       ` Junio C Hamano

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

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

  git send-email \
    --in-reply-to=an2IT78KDS94JqUt@pks.im \
    --to=ps@pks.im \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=github@paulisageek.com \
    --cc=gitster@pobox.com \
    --cc=koji.nakamaru@gree.net \
    --cc=me@ttaylorr.com \
    --cc=peff@peff.net \
    --cc=tamird@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.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox