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
next prev parent 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