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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.