Re: [PATCH v5 09/15] index-helper: use watchman to avoid refreshing index with lstat()

On Wed, Apr 20, 2016 at 8:01 AM, David Turner [off-list ref] wrote:

On Wed, 2016-04-20 at 07:15 +0700, Duy Nguyen wrote:

quoted

Continuing my comment from the --use-watchman patch about watchman
not
being supported...

On Wed, Apr 20, 2016 at 6:28 AM, David Turner <
dturner@twopensource.com> wrote:

quoted

+static int poke_and_wait_for_reply(int fd)
+{
+       struct strbuf buf = STRBUF_INIT;
+       struct strbuf reply = STRBUF_INIT;
+       int ret = -1;
+       fd_set fds;
+       struct timeval timeout;
+
+       timeout.tv_usec = 0;
+       timeout.tv_sec = 1;
+
+       if (fd < 0)
+               return -1;
+
+       strbuf_addf(&buf, "poke %d", getpid());
+       if (write_in_full(fd, buf.buf, buf.len + 1) != buf.len + 1)
+               goto done_poke;
+
+       /* Now wait for a reply */
+       FD_ZERO(&fds);
+       FD_SET(fd, &fds);
+       if (select(fd + 1, &fds, NULL, NULL, &timeout) == 0)
+               /* No reply, giving up */
+               goto done_poke;
+
+       if (strbuf_getwholeline_fd(&reply, fd, 0))
+               goto done_poke;
+
+       if (!starts_with(reply.buf, "OK"))
+               goto done_poke;

... while we could simply check USE_WATCHMAN macro and reject in
update-index, a better solution is sending "poke %d watchman" and
returning "OK watchman" (vs "OK") when watchman is supported and
active. If the user already requests watchman and index-helper
returns
just "OK" then we can warn the user the reason of possible
performance
degradation. It's related to the error reporting, but I don't think
you can send straight errors over unix socket. It's possible but it's
a bit more complicated.

Do you mean that we should do this here?  Or in update-index -
-watchman?  If the former, I agree.  If the latter, I'm not sure; maybe
you'll be setting up your index before you've started the index helper?

Here is better than update-index because we can't know what
index-helper is capable of (the USE_WATCHMAN macro is more like a
suggestion)

quoted

quoted

+static void refresh_by_watchman(struct index_state *istate)
+{
+       void *shm = NULL;
+       int length;
+       int i;
+       struct stat st;
+       int fd = -1;
+       const char *path = git_path("shm-watchman-%s-%"PRIuMAX,
+                                   sha1_to_hex(istate->sha1),
+                                   (uintmax_t)getpid());
+
+       fd = open(path, O_RDONLY);
+       if (fd < 0)
+               return;
+
+       /*
+        * This watchman data is just for us -- no need to keep it
+        * around once we've got it open.
+        */
+       unlink(path);

This will not play well when multiple processes read and refresh the
index at the same time.

Multiple processes will have different pids, right?  And the pid is
included in the filename.  Am I missing something?

Ahhh! I thought that pid was index-helper's. Silly me.

quoted

Now that I think of it, with watchman backing us, we probably should
just do nothing in update_index_if_able() (or write_locked_index()
when we know only stat info is changed) when watchman is active. The
purpose of update_index_if_able() is to avoid costly refresh, but we
can already avoid that with watchman. And updating big index files is
always costly (even though it should cost less with split-index).

That sounds like a change we could make in a separate series.  It's not
a bad idea, but if our goal is to get the basic version out, we should
start there.

Agreed. More optimizations can always wait till later. We just need a
good foundation first.
-- 
Duy