git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v6 03/19] index-helper: new daemon for caching index and related stuff

From
Junio C Hamano <gitster@pobox.com>
Date
Apr 28, 2016, 18:58 UTC
Message-ID
<xmqq60v15zmq.fsf@gitster.mtv.corp.google.com>
In-Reply-To
<1461787481-877-4-git-send-email-dturner@twopensource.com>
David Turner <dturner@twopensource.com> writes:
Show 5 quoted lines
> From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ...
> The biggest gain is not having to verify the trailing SHA-1, which
> takes lots of time especially on large index files. But this also
> opens doors for further optimiztions:
optimizAtion
Show 5 quoted lines
> Git can poke the daemon via unix domain sockets to tell it to refresh
> the index cache, or to keep it alive some more minutes. It can't give
> any real index data directly to the daemon. Real data goes to disk
> first, then the daemon reads and verifies it from there. Poking only
> happens for $GIT_DIR/index, not temporary index files.

Is this limited to "poking", or the helper daemon is not involved in codepaths that handle temporary index at all? It makes sense if it is the latter, and it doesn't if it were the former, but it is unclear in this paragraph.

Show 5 quoted lines
> $GIT_DIR/index-helper.sock is the socket for the daemon process. The
> daemon reads from the socket and executes commands.
>
> Named pipes were considered for portability reasons, but then commands
> that need replies from the daemon would have open their own pipes,
"would have TO open"?
> since a named pipe should only have one reader.  Unix domain sockets
> don't have this problem.
Show 22 quoted lines
> diff --git a/Documentation/git-index-helper.txt b/Documentation/git-index-helper.txt
> new file mode 100644
> index 0000000..77687c0
> --- /dev/null
> +++ b/Documentation/git-index-helper.txt
> @@ -0,0 +1,47 @@
> +git-index-helper(1)
> +===================
> +
> +NAME
> +----
> +git-index-helper - A simple cache daemon for speeding up index file access
> +
> +SYNOPSIS
> +--------
> +[verse]
> +'git index-helper' [options]
> +
> +DESCRIPTION
> +-----------
> +Keep the index file in memory for faster access. This daemon is per
> +repository.
Not an objection but a question.  Does this mean "per index file"?

What the users would need to be aware of is that it is not like running a single daemon instance helps the toplevel project with its entire 600 submodules checked out you have--you would need that many instances of the helper, which is already conveyed well with "per repository". But I was wondering if it helps users experimenting with the multiple worktree feature if we said "per index file". It would make it more clear that you would run two instances of the helper daemon when you use another worktree in addition to your primary repository.

Show 10 quoted lines
> +... The following commands are used to control the daemon:
> +
> +"refresh"::
> +	Reread the index.
> +
> +"poke":
> +	Let the daemon know the index is to be read. It keeps the
> +	daemon alive longer, unless `--exit-after=0` is used.
> +
> +All commands and replies are terminated by a 0 byte.
s/0/NUL/
Show 10 quoted lines
> diff --git a/cache.h b/cache.h
> index 4180e2b..43fb314 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -334,6 +334,8 @@ struct index_state {
>  	struct cache_time timestamp;
>  	unsigned name_hash_initialized : 1,
>  		 keep_mmap : 1,
> +		 from_shm : 1,
> +		 to_shm : 1,

keep_mmap bit had its own commit, which made it more-or-less unnecessary to have a separate explanation, but it is unclear how these two bits are envisioned to be used...

Show 50 quoted lines
> diff --git a/index-helper.c b/index-helper.c
> new file mode 100644
> index 0000000..976e913
> --- /dev/null
> +++ b/index-helper.c
> @@ -0,0 +1,285 @@
> +#include "cache.h"
> +#include "parse-options.h"
> +#include "sigchain.h"
> +#include "strbuf.h"
> +#include "exec_cmd.h"
> +#include "split-index.h"
> +#include "lockfile.h"
> +#include "cache.h"
> +#include "unix-socket.h"
> +#include "pkt-line.h"
> +
> +struct shm {
> +	unsigned char sha1[20];
> +	void *shm;
> +	size_t size;
> +};
> +
> +static struct shm shm_index;
> +static struct shm shm_base_index;
> +
> +static void release_index_shm(struct shm *is)
> +{
> +	if (!is->shm)
> +		return;
> +	munmap(is->shm, is->size);
> +	unlink(git_path("shm-index-%s", sha1_to_hex(is->sha1)));
> +	is->shm = NULL;
> +}
> +
> +static void cleanup_shm(void)
> +{
> +	release_index_shm(&shm_index);
> +	release_index_shm(&shm_base_index);
> +}
> +
> +static void cleanup(void)
> +{
> +	unlink(git_path("index-helper.sock"));
> +	cleanup_shm();
> +}
> +
> +static void cleanup_on_signal(int sig)
> +{
> +	/* We ignore sigpipes -- that's just a client being broken. */

Is it really "a broken" client, in which case we would want to know about the breakage so that we can fix it? Or is it a case of "a client wasn't interested in what we had to say, which is perfectly fine"?

Show 11 quoted lines
> +	if (sig == SIGPIPE)
> +		return;
> +	cleanup();
> +	sigchain_pop(sig);
> +	raise(sig);
> +}
> +
> +static int shared_mmap_create(int file_flags, int file_mode, size_t size,
> +			      void **new_mmap, int mmap_prot, int mmap_flags,
> +			      const char *path)
> +{

I can understand size, new_mmap and path, but do all others need the customizability? It is hard to tell at this point in the series as there is only one caller, which gives a reasonable set of values to these parameters. And each of all these "reasonable values" is the only reasonable value for each of these parameters-- you would ever want to have mmap_prot without PROT_WRIT as this is creating an empty region that is mapped to a newly created file, for example. It is inconceivable that a function named "shared_mmap_create" ever passes mmap_flags that lack MAP_SHARED, for another example, or file_flags that is not O_CREAT|O_EXCL|O_RDWR for that matter.

Show 22 quoted lines
> + ...
> +	if (ftruncate(fd, size))
> +		goto done;
> +	*new_mmap = mmap(NULL, size, mmap_prot, mmap_flags, fd, 0);
> ...
> +static void share_index(struct index_state *istate, struct shm *is)
> +{
> +	void *new_mmap;
> +	if (istate->mmap_size <= 20 ||
> +	    hashcmp(istate->sha1,
> +		    (unsigned char *)istate->mmap + istate->mmap_size - 20) ||
> +	    !hashcmp(istate->sha1, is->sha1) ||
> +	    shared_mmap_create(O_CREAT | O_EXCL | O_RDWR, 0700,
> +			       istate->mmap_size, &new_mmap,
> +			       PROT_READ | PROT_WRITE, MAP_SHARED,
> +			       git_path("shm-index-%s",
> +					sha1_to_hex(istate->sha1))) < 0)
> +		return;
> +	release_index_shm(is);
> +	is->size = istate->mmap_size;
> +	is->shm = new_mmap;
> +	hashcpy(is->sha1, istate->sha1);

So "is" is expected to be a valid instance of shm that was obtained previously. When we fail to map and return, does it make sense to retain it? Because this function does not return any value, the caller wouldn't be able to tell that shared-mmap-create failed, and will keep using that stale instance of "is"--I cannot tell if that is OK in the caller of this thing or not from what I read so far in this series...

Show 5 quoted lines
> +static void refresh(void)
> +{
> +	discard_index(&the_index);
> +	the_index.keep_mmap = 1;
> +	the_index.to_shm    = 1;
Again, mysterious to_shm appears here...
> +		/* Wait for a request */
> +		FD_ZERO(&readfds);
> +		FD_SET(fd, &readfds);
> +		result = select(fd + 1, &readfds, NULL, NULL, timeout_p);

I admit that I am old fashioned and am more familiar with select(2) than poll(2), but I think we try to avoid select(2) in new code.

Previous: David TurnerNext: David Turner
Message 5 of 24 in “index-helper/watchman”
  1. 00/19 index-helper/watchmanDavid Turner, Apr 27, 2016
  2. 01/19 read-cache.c: fix constness of verify_hdr()David Turner, Apr 27, 2016
  3. 02/19 read-cache: allow to keep mmap'd memory after readingDavid Turner, Apr 27, 2016
  4. 03/19 index-helper: new daemon for caching index and related stuffDavid Turner, Apr 27, 2016
  5. Junio C HamanoApr 28, 2016
  6. David TurnerApr 29, 2016
  7. Junio C HamanoApr 29, 2016
  8. Duy NguyenMay 1, 2016
  9. 04/19 index-helper: add --strictDavid Turner, Apr 27, 2016
  10. 05/19 index-helper: log warningsDavid Turner, Apr 27, 2016
  11. 06/19 daemonize(): set a flag before exiting the main processDavid Turner, Apr 27, 2016
  12. 07/19 index-helper: add --detachDavid Turner, Apr 27, 2016
  13. 08/19 read-cache: add watchman 'WAMA' extensionDavid Turner, Apr 27, 2016
  14. 09/19 Add watchman support to reduce index refresh costDavid Turner, Apr 27, 2016
  15. 10/19 index-helper: use watchman to avoid refreshing index with lstat()David Turner, Apr 27, 2016
  16. 11/19 update-index: enable/disable watchman supportDavid Turner, Apr 27, 2016
  17. 12/19 unpack-trees: preserve index extensionsDavid Turner, Apr 27, 2016
  18. 13/19 watchman: add a config option to enable the extensionDavid Turner, Apr 27, 2016
  19. 14/19 index-helper: kill modeDavid Turner, Apr 27, 2016
  20. 15/19 index-helper: don't run if already runningDavid Turner, Apr 27, 2016
  21. 16/19 index-helper: autorun modeDavid Turner, Apr 27, 2016
  22. 17/19 index-helper: optionally automatically runDavid Turner, Apr 27, 2016
  23. 18/19 Add tracing to measure where most of the time is spentDavid Turner, Apr 27, 2016
  24. 19/19 untracked-cache: config optionDavid Turner, Apr 27, 2016

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.