Re: [PATCH v5 09/15] index-helper: use watchman to avoid refreshing index with lstat()
- From
Duy Nguyen <pclouds@gmail.com>
- Date
- Apr 20, 2016, 00:15 UTC
- Message-ID
- <CACsJy8CM409OH12w3EdVP3UjXoURbWNuqb_coQ=AagdCs+ctaQ@mail.gmail.com>
- In-Reply-To
- <1461108489-29376-10-git-send-email-dturner@twopensource.com>
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:
Show 30 quoted lines
> +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.
Show 20 quoted lines
> +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. They could refresh non-overlapping subdirectories, and I think it's perfectly ok for them to do so (writing index down is a different matter). I don't have a good answer for this. Perhaps if shm-watchman-%s-%d file is small enough (and it should be, we store it in the index), then we can just send the content straight over unix socket. I didn't have this option with my signal-based communication model.
This is really extra. But if we know in advance that git does not need refresh(), then we should be able to tell index-helper not to waste cycles contacting watchman and preparing shm-watchman-%s-%d (the poke line gets more parameters). Either that, or we decouple watchman requests from read_cache() requests. Only when refresh_index() is called that we send something to request shm-watchman-%s-%d. The same for read_directory() (i.e. untracked cache stuff). Hmm?
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). Of course this can only be done if watchman (inotify to be precise) can cover whole worktree. I'm not sure how watchman behaves when there's not enough inotify resource to cover full worktree.
-- Duy