{"thread":{"id":"37931","subject":"[RFC] On watchman support","startedAt":"2014-11-11T12:49:01Z","lastAt":"2014-12-01T20:45:50Z","messageCount":15,"participants":["Duy Nguyen","Torsten Bögershausen","David Turner","Junio C Hamano","Jeff King","Paolo Ciarrocchi"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"251714","messageId":"20141111124901.GA6011@lanh","threadId":"37931","inReplyTo":null,"subject":"[RFC] On watchman support","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-11-11T12:49:01Z","receivedAt":"2014-11-11T12:49:01Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"I've come to the last piece to speed up \"git status\", watchman\nsupport. And I realized it's not as good as I thought.\n\nWatchman could be used for two things: to avoid refreshing the index,\nand to avoid searching for ignored files. The first one can be done\n(with the patch below as demonstration). And it should keep refresh\ncost to near zero in the best case, the cost is proportional to the\nnumber of modified files.\n\nFor avoiding searching for ignored files. My intention was to build on\ntop of untracked cache. If watchman can tell me what files are added\nor deleted since last observed time, then I can invalidate just\ndirectories that contain them, or even better, calculate ignore status\nfor those files only.\n\nThis is important because in reality compilers and editors tend to\nupdate files by creating a new version then rename them, updating\ndirectory mtime and invalidating untracked cache as a consequence. As\nyou edit more files (or your rebuild touches more dirs), untracked\ncache performance drops (until the next \"git status\"). The numbers I\nposted so far are the best case.\n\nThe problem with watchman is it cannot tell me \"new\" files since the\nlast observed time (let's say 'T'). If a file exists at 'T', gets\ndeleted then recreated, then watchman tells me it's a new file. I want\nto separate those from ones that do not exist before 'T'.\n\nDavid's watchman approach does not have this problem because he keeps\ntrack of all entries under $GIT_WORK_TREE and knows which files are\ntruely new. But I don't really want to keep the whole file list around,\nespecially when watchman already manages the same list.\n\nSo we got a few options:\n\n1) Convince watchman devs to add something to make it work\n\n2) Fork watchman\n\n3) Make another daemon to keep file list around, or put it in a shared\n   memory.\n\n4) Move David's watchman series forward (and maybe make use of shared\n   mem for fs_cache).\n\n5) Go with something similar to the patch below and accept untracked\n   cache performance degrades from time to time\n\n6) ??\n\nI'm working on 1). 2) is just bad taste, listed for completeness\nonly. If we go with 3) and watchman starts to support Windows (seems\nto be in their plan), we'll need to rework some how. And I really\ndon't like 3)\n\nIf 1-3 does not work out, we're left without 4) and 5). We could\nsupport both, but proobably not worth the code complexity and should\njust go with one.\n\nAnd if we go with 4) we should probably think of dropping untracked\ncache if watchman will support Windows in the end. 4) also has another\nadvantage over untracked cache, that it could speed up listing ignored\nfiles as well as untracked files.\n\nComments?\n\n-- 8< --\ndiff --git a/Makefile b/Makefile\nindex fa58a53..a2be728 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -406,6 +406,7 @@ TCLTK_PATH = wish\n XGETTEXT = xgettext\n MSGFMT = msgfmt\n PTHREAD_LIBS = -lpthread\n+WATCHMAN_LIBS = -lwatchman\n PTHREAD_CFLAGS =\n GCOV = gcov\n \n@@ -1453,6 +1454,13 @@ else\n \tLIB_OBJS += thread-utils.o\n endif\n \n+ifdef USE_WATCHMAN\n+\tLIB_H += watchman-support.h\n+\tLIB_OBJS += watchman-support.o\n+\tEXTLIBS += $(WATCHMAN_LIBS)\n+\tBASIC_CFLAGS += -DUSE_WATCHMAN\n+endif\n+\n ifdef HAVE_PATHS_H\n \tBASIC_CFLAGS += -DHAVE_PATHS_H\n endif\n@@ -2222,6 +2230,7 @@ GIT-BUILD-OPTIONS: FORCE\n \t@echo NO_PERL=\\''$(subst ','\\'',$(subst ','\\'',$(NO_PERL)))'\\' >>$@\n \t@echo NO_PYTHON=\\''$(subst ','\\'',$(subst ','\\'',$(NO_PYTHON)))'\\' >>$@\n \t@echo NO_UNIX_SOCKETS=\\''$(subst ','\\'',$(subst ','\\'',$(NO_UNIX_SOCKETS)))'\\' >>$@\n+\t@echo USE_WATCHMAN=\\''$(subst ','\\'',$(subst ','\\'',$(USE_WATCHMAN)))'\\' >>$@\n ifdef TEST_OUTPUT_DIRECTORY\n \t@echo TEST_OUTPUT_DIRECTORY=\\''$(subst ','\\'',$(subst ','\\'',$(TEST_OUTPUT_DIRECTORY)))'\\' >>$@\n endif\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex f23ec83..1da2b15 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -882,6 +882,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n {\n \tint newfd, entries, has_errors = 0, line_termination = '\\n';\n \tint untracked_cache = -1;\n+\tint use_watchman = -1;\n \tint read_from_stdin = 0;\n \tint prefix_length = prefix ? strlen(prefix) : 0;\n \tint preferred_index_format = 0;\n@@ -977,6 +978,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \t\t\tN_(\"enable/disable untracked cache\")),\n \t\tOPT_SET_INT(0, \"force-untracked-cache\", &untracked_cache,\n \t\t\t    N_(\"enable untracked cache without testing the filesystem\"), 2),\n+\t\tOPT_BOOL(0, \"watchman\", &use_watchman,\n+\t\t\tN_(\"use or not use watchman to reduce refresh cost\")),\n \t\tOPT_END()\n \t};\n \n@@ -1102,6 +1105,14 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \t\tthe_index.cache_changed |= UNTRACKED_CHANGED;\n \t}\n \n+\tif (use_watchman > 0) {\n+\t\tthe_index.last_update    = xstrdup(\"\");\n+\t\tthe_index.cache_changed |= WATCHMAN_CHANGED;\n+\t} else if (!use_watchman) {\n+\t\tthe_index.last_update    = NULL;\n+\t\tthe_index.cache_changed |= WATCHMAN_CHANGED;\n+\t}\n+\n \tif (active_cache_changed) {\n \t\tif (newfd < 0) {\n \t\t\tif (refresh_args.flags & REFRESH_QUIET)\ndiff --git a/cache.h b/cache.h\nindex 201b22e..e1d6e21 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -161,6 +161,8 @@ struct cache_entry {\n #define CE_VALID     (0x8000)\n #define CE_STAGESHIFT 12\n \n+#define CE_NO_WATCH  (0x0001)\n+\n /*\n  * Range 0xFFFF0FFF in ce_flags is divided into\n  * two parts: in-memory flags and on-disk ones.\n@@ -296,6 +298,7 @@ static inline unsigned int canon_mode(unsigned int mode)\n #define CACHE_TREE_CHANGED\t(1 << 5)\n #define SPLIT_INDEX_ORDERED\t(1 << 6)\n #define UNTRACKED_CHANGED       (1 << 7)\n+#define WATCHMAN_CHANGED\t(1 << 8)\n \n struct split_index;\n struct untracked_cache;\n@@ -314,6 +317,7 @@ struct index_state {\n \tstruct hashmap dir_hash;\n \tunsigned char sha1[20];\n \tstruct untracked_cache *untracked;\n+\tchar *last_update;\n };\n \n extern struct index_state the_index;\n@@ -637,6 +641,8 @@ extern int check_replace_refs;\n \n extern int fsync_object_files;\n extern int core_preload_index;\n+extern int core_use_watchman;\n+extern int core_watchman_sync_timeout;\n extern int core_apply_sparse_checkout;\n extern int precomposed_unicode;\n \ndiff --git a/config.c b/config.c\nindex 9e42d38..72b9223 100644\n--- a/config.c\n+++ b/config.c\n@@ -860,6 +860,16 @@ static int git_default_core_config(const char *var, const char *value)\n \t\treturn 0;\n \t}\n \n+\tif (!strcmp(var, \"core.usewatchman\")) {\n+\t\tcore_use_watchman = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\n+\tif (!strcmp(var, \"core.watchmansynctimeout\")) {\n+\t\tcore_watchman_sync_timeout = git_config_int(var, value);\n+\t\treturn 0;\n+\t}\n+\n \tif (!strcmp(var, \"core.createobject\")) {\n \t\tif (!strcmp(value, \"rename\"))\n \t\t\tobject_creation_mode = OBJECT_CREATION_USES_RENAMES;\ndiff --git a/configure.ac b/configure.ac\nindex 4b1ae7c..2ee5356 100644\n--- a/configure.ac\n+++ b/configure.ac\n@@ -970,6 +970,12 @@ GIT_CONF_SUBST([NO_INITGROUPS])\n #\n # Define NO_ICONV if your libc does not properly support iconv.\n \n+# Check for watchman client library\n+\n+AC_CHECK_LIB([watchman], [watchman_connect],\n+\t[USE_WATCHMAN=YesPlease],\n+\t[USE_WATCHMAN=])\n+GIT_CONF_SUBST([USE_WATCHMAN])\n \n ## Other checks.\n # Define USE_PIC if you need the main git objects to be built with -fPIC\ndiff --git a/environment.c b/environment.c\nindex 565f652..dfaea4e 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -74,6 +74,11 @@ int auto_comment_line_char;\n /* Parallel index stat data preload? */\n int core_preload_index = 1;\n \n+/* Use Watchman for faster status queries */\n+int core_use_watchman = 0;\n+int core_watchman_sync_timeout = 300;\n+\n+\n /* This is set by setup_git_dir_gently() and/or git_default_config() */\n char *git_work_tree_cfg;\n static char *work_tree;\ndiff --git a/read-cache.c b/read-cache.c\nindex 21ae963..46566f5 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -16,6 +16,7 @@\n #include \"varint.h\"\n #include \"split-index.h\"\n #include \"sigchain.h\"\n+#include \"ewah/ewok.h\"\n \n static struct cache_entry *refresh_cache_entry(struct cache_entry *ce,\n \t\t\t\t\t       unsigned int options);\n@@ -38,11 +39,13 @@ static struct cache_entry *refresh_cache_entry(struct cache_entry *ce,\n #define CACHE_EXT_RESOLVE_UNDO 0x52455543 /* \"REUC\" */\n #define CACHE_EXT_LINK 0x6c696e6b\t  /* \"link\" */\n #define CACHE_EXT_UNTRACKED 0x554E5452\t  /* \"UNTR\" */\n+#define CACHE_EXT_WATCHMAN 0x57414D41\t  /* \"WAMA\" */\n \n /* changes that can be kept in $GIT_DIR/index (basically all extensions) */\n #define EXTMASK (RESOLVE_UNDO_CHANGED | CACHE_TREE_CHANGED | \\\n \t\t CE_ENTRY_ADDED | CE_ENTRY_REMOVED | CE_ENTRY_CHANGED | \\\n-\t\t SPLIT_INDEX_ORDERED | UNTRACKED_CHANGED)\n+\t\t SPLIT_INDEX_ORDERED | UNTRACKED_CHANGED | \\\n+\t\t WATCHMAN_CHANGED)\n \n struct index_state the_index;\n static const char *alternate_index_output;\n@@ -1217,8 +1220,13 @@ int refresh_index(struct index_state *istate, unsigned int flags,\n \t\t\tcontinue;\n \n \t\tnew = refresh_cache_ent(istate, ce, options, &cache_errno, &changed);\n-\t\tif (new == ce)\n+\t\tif (new == ce) {\n+\t\t\tif (ce->ce_flags & CE_NO_WATCH) {\n+\t\t\t\tce->ce_flags          &= ~CE_NO_WATCH;\n+\t\t\t\tistate->cache_changed |= WATCHMAN_CHANGED;\n+\t\t\t}\n \t\t\tcontinue;\n+\t\t}\n \t\tif (!new) {\n \t\t\tconst char *fmt;\n \n@@ -1370,6 +1378,55 @@ static int verify_hdr(struct cache_header *hdr, unsigned long size)\n \treturn 0;\n }\n \n+static void mark_no_watchman(size_t pos, void *data)\n+{\n+\tstruct index_state *istate = data;\n+\tassert(pos < istate->cache_nr);\n+\tistate->cache[pos]->ce_flags |= CE_NO_WATCH;\n+}\n+\n+static int read_watchman_ext(struct index_state *istate, const void *data,\n+\t\t\t      unsigned long sz)\n+{\n+\tstruct ewah_bitmap *bitmap;\n+\tint ret, len;\n+\n+\tif (memchr(data, 0, sz) == NULL)\n+\t\treturn error(\"invalid extension\");\n+\tlen = strlen(data) + 1;\n+\tbitmap = ewah_new();\n+\tret = ewah_read_mmap(bitmap, (const char *)data + len, sz - len);\n+\tif (ret != sz - len) {\n+\t\tewah_free(bitmap);\n+\t\treturn error(\"fail to parse ewah bitmap\");\n+\t}\n+\tistate->last_update = xstrdup(data);\n+\tewah_each_bit(bitmap, mark_no_watchman, istate);\n+\tewah_free(bitmap);\n+\treturn 0;\n+}\n+\n+static int write_strbuf(void *user_data, const void *data, size_t len)\n+{\n+\tstruct strbuf *sb = user_data;\n+\tstrbuf_add(sb, data, len);\n+\treturn len;\n+}\n+\n+static void write_watchman_ext(struct strbuf *sb, struct index_state* istate)\n+{\n+\tstruct ewah_bitmap *bitmap;\n+\tint i;\n+\n+\tstrbuf_add(sb, istate->last_update, strlen(istate->last_update) + 1);\n+\tbitmap = ewah_new();\n+\tfor (i = 0; i < istate->cache_nr; i++)\n+\t\tif (istate->cache[i]->ce_flags & CE_NO_WATCH)\n+\t\t\tewah_set(bitmap, i);\n+\tewah_serialize_to(bitmap, write_strbuf, sb);\n+\tewah_free(bitmap);\n+}\n+\n static int read_index_extension(struct index_state *istate,\n \t\t\t\tconst char *ext, void *data, unsigned long sz)\n {\n@@ -1387,6 +1444,11 @@ static int read_index_extension(struct index_state *istate,\n \tcase CACHE_EXT_UNTRACKED:\n \t\tistate->untracked = read_untracked_extension(data, sz);\n \t\tbreak;\n+\n+\tcase CACHE_EXT_WATCHMAN:\n+\t\tread_watchman_ext(istate, data, sz);\n+\t\tbreak;\n+\n \tdefault:\n \t\tif (*ext < 'A' || 'Z' < *ext)\n \t\t\treturn error(\"index uses %.4s extension, which we do not understand\",\n@@ -1600,10 +1662,10 @@ int read_index_from(struct index_state *istate, const char *path)\n \tret = do_read_index(istate, path, 0);\n \tsplit_index = istate->split_index;\n \tif (!split_index)\n-\t\treturn ret;\n+\t\tgoto done;\n \n \tif (is_null_sha1(split_index->base_sha1))\n-\t\treturn ret;\n+\t\tgoto done;\n \n \tif (split_index->base)\n \t\tdiscard_index(split_index->base);\n@@ -1619,6 +1681,12 @@ int read_index_from(struct index_state *istate, const char *path)\n \t\t\t\t     sha1_to_hex(split_index->base_sha1)),\n \t\t    sha1_to_hex(split_index->base->sha1));\n \tmerge_base_index(istate);\n+\n+done:\n+#ifdef USE_WATCHMAN\n+\tif (istate->last_update && !getenv(\"GIT_NO_WATCHMAN\"))\n+\t\tcheck_watchman(istate);\n+#endif\n \treturn ret;\n }\n \n@@ -1654,6 +1722,8 @@ int discard_index(struct index_state *istate)\n \tdiscard_split_index(istate);\n \tfree_untracked_cache(istate->untracked);\n \tistate->untracked = NULL;\n+\tfree(istate->last_update);\n+\tistate->last_update = NULL;\n \treturn 0;\n }\n \n@@ -2051,6 +2121,16 @@ static int do_write_index(struct index_state *istate, int newfd,\n \t\tif (err)\n \t\t\treturn -1;\n \t}\n+\tif (istate->last_update) {\n+\t\tstruct strbuf sb = STRBUF_INIT;\n+\n+\t\twrite_watchman_ext(&sb, istate);\n+\t\terr = write_index_ext_header(&c, newfd, CACHE_EXT_WATCHMAN, sb.len) < 0\n+\t\t\t|| ce_write(&c, newfd, sb.buf, sb.len) < 0;\n+\t\tstrbuf_release(&sb);\n+\t\tif (err)\n+\t\t\treturn -1;\n+\t}\n \n \tif (ce_flush(&c, newfd, istate->sha1) || fstat(newfd, &st))\n \t\treturn -1;\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex b1bc65b..0080f47 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -897,6 +897,7 @@ test -z \"$NO_PERL\" && test_set_prereq PERL\n test -z \"$NO_PYTHON\" && test_set_prereq PYTHON\n test -n \"$USE_LIBPCRE\" && test_set_prereq LIBPCRE\n test -z \"$NO_GETTEXT\" && test_set_prereq GETTEXT\n+test -z \"$USE_WATCHMAN\" && test_set_prereq WATCHMAN\n \n # Can we rely on git's output in the C locale?\n if test -n \"$GETTEXT_POISON\"\ndiff --git a/watchman-support.c b/watchman-support.c\nnew file mode 100644\nindex 0000000..f608457\n--- /dev/null\n+++ b/watchman-support.c\n@@ -0,0 +1,132 @@\n+#include \"cache.h\"\n+#include \"watchman-support.h\"\n+#include \"strbuf.h\"\n+#include <watchman.h>\n+\n+static struct watchman_query *make_query(const char *last_update)\n+{\n+\tstruct watchman_query *query = watchman_query();\n+\twatchman_query_set_fields(query, WATCHMAN_FIELD_NAME |\n+\t\t\t\t\t WATCHMAN_FIELD_EXISTS |\n+\t\t\t\t\t WATCHMAN_FIELD_NEWER);\n+\t/* watchman_query_set_fields(query, WATCHMAN_FIELD_CCLOCK); */\n+\twatchman_query_set_empty_on_fresh(query, 1);\n+\tquery->sync_timeout = core_watchman_sync_timeout;\n+\tif (*last_update)\n+\t\twatchman_query_set_since_oclock(query, last_update);\n+\treturn query;\n+}\n+\n+static struct watchman_query_result* query_watchman(\n+\tstruct index_state *istate, struct watchman_connection *connection,\n+\tconst char *fs_path, const char *last_update)\n+{\n+\tstruct watchman_error wm_error;\n+\tstruct watchman_query *query;\n+\tstruct watchman_expression *expr;\n+\tstruct watchman_query_result *result;\n+\tstruct stat st;\n+\n+\tif (lstat(get_git_dir(), &st)) {\n+\t\t/*\n+\t\t * Watchman gets confused if we delete the .git\n+\t\t * directory out from under it, since that's where it\n+\t\t * stores its cookies.  So we'll need to delete the\n+\t\t * watch and then recreate it. It's OK for this to\n+\t\t * fail, as the watch might have already been deleted.\n+\t\t */\n+\t\twatchman_watch_del(connection, fs_path, &wm_error);\n+\n+\t\tif (watchman_watch(connection, fs_path, &wm_error)) {\n+\t\t\twarning(\"Watchman watch error: %s\", wm_error.message);\n+\t\t\treturn NULL;\n+\t\t}\n+\t}\n+\n+\tquery = make_query(last_update);\n+\texpr = watchman_true_expression();\n+\tresult = watchman_do_query(connection, fs_path, query, expr, &wm_error);\n+\twatchman_free_query(query);\n+\twatchman_free_expression(expr);\n+\n+\tif (!result)\n+\t\twarning(\"Watchman query error: %s (at %s)\",\n+\t\t\twm_error.message,\n+\t\t\t*last_update ? last_update : \"the beginning\");\n+\n+\treturn result;\n+}\n+\n+static void update_index(struct index_state *istate,\n+\t\t\t struct watchman_query_result *result)\n+{\n+\tint i;\n+\n+\tif (result->is_fresh_instance) {\n+\t\t/* let refresh clear them later */\n+\t\tfor (i = 0; i < istate->cache_nr; i++)\n+\t\t\tistate->cache[i]->ce_flags |= CE_NO_WATCH;\n+\t\tgoto done;\n+\t}\n+\n+\tfor (i = 0; i < result->nr; i++) {\n+\t\tstruct watchman_stat *wm = result->stats + i;\n+\t\tint pos;\n+\n+\t\tif (!strncmp(wm->name, \".git/\", 5) ||\n+\t\t    strstr(wm->name, \"/.git/\"))\n+\t\t\tcontinue;\n+\n+\t\tpos = index_name_pos(istate, wm->name, strlen(wm->name));\n+\t\tif (pos < 0)\n+\t\t\tcontinue;\n+\t\t/* FIXME: ignore staged entries and gitlinks too? */\n+\n+\t\tistate->cache[pos]->ce_flags |= CE_NO_WATCH;\n+\t}\n+\n+\t/*\n+\t * we have marked all modified files with CE_NO_WATCH\n+\t * according to watchman. All other files are supposed to be\n+\t * uptodate. Those with CE_NO_WATCH will be refreshed\n+\t * eventually and get that bit cleared.\n+\t */\n+\tfor (i = 0; i < istate->cache_nr; i++) {\n+\t\tstruct cache_entry *ce = istate->cache[i];\n+\t\tif (ce_stage(ce) || (ce->ce_flags & CE_NO_WATCH))\n+\t\t\tcontinue;\n+\t\tce_mark_uptodate(ce);\n+\t}\n+\n+done:\n+\tfree(istate->last_update);\n+\tistate->last_update    = xstrdup(result->clock);\n+\tistate->cache_changed |= WATCHMAN_CHANGED;\n+}\n+\n+int check_watchman(struct index_state *istate)\n+{\n+\tstruct watchman_error wm_error;\n+\tstruct watchman_connection *connection;\n+\tstruct watchman_query_result *result;\n+\tconst char *fs_path;\n+\n+\tfs_path = get_git_work_tree();\n+\tif (!fs_path)\n+\t\treturn -1;\n+\n+\tconnection = watchman_connect(&wm_error);\n+\n+\tif (!connection) {\n+\t\twarning(\"Watchman watch error: %s\", wm_error.message);\n+\t\treturn -1;\n+\t}\n+\n+\tresult = query_watchman(istate, connection, fs_path, istate->last_update);\n+\twatchman_connection_close(connection);\n+\tif (!result)\n+\t\treturn -1;\n+\tupdate_index(istate, result);\n+\twatchman_free_query_result(result);\n+\treturn 0;\n+}\ndiff --git a/watchman-support.h b/watchman-support.h\nnew file mode 100644\nindex 0000000..5610409\n--- /dev/null\n+++ b/watchman-support.h\n@@ -0,0 +1,8 @@\n+#ifndef WATCHMAN_SUPPORT_H\n+#define WATCHMAN_SUPPORT_H\n+\n+struct index_state;\n+int check_watchman(struct index_state *index);\n+\n+\n+#endif /* WATCHMAN_SUPPORT_H */\n-- 8< --\n"},{"id":"251789","messageId":"54643C30.6010204@web.de","threadId":"37931","inReplyTo":"20141111124901.GA6011@lanh","subject":"Re: [RFC] On watchman support","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2014-11-13T05:05:52Z","receivedAt":"2014-11-13T05:05:52Z","isPatch":false,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2014-11-11 13.49, Duy Nguyen wrote:\n> I've come to the last piece to speed up \"git status\", watchman\n> support. And I realized it's not as good as I thought.\n> \n> Watchman could be used for two things: to avoid refreshing the index,\n> and to avoid searching for ignored files. The first one can be done\n> (with the patch below as demonstration). And it should keep refresh\n> cost to near zero in the best case, the cost is proportional to the\n> number of modified files.\n> \n> For avoiding searching for ignored files. My intention was to build on\n> top of untracked cache. If watchman can tell me what files are added\n> or deleted since last observed time, then I can invalidate just\n> directories that contain them, or even better, calculate ignore status\n> for those files only.\n> \n> This is important because in reality compilers and editors tend to\n> update files by creating a new version then rename them, updating\n> directory mtime and invalidating untracked cache as a consequence. As\n> you edit more files (or your rebuild touches more dirs), untracked\n> cache performance drops (until the next \"git status\"). The numbers I\n> posted so far are the best case.\n> \n> The problem with watchman is it cannot tell me \"new\" files since the\n> last observed time (let's say 'T'). If a file exists at 'T', gets\n> deleted then recreated, then watchman tells me it's a new file. I want\n> to separate those from ones that do not exist before 'T'.\n> \n> David's watchman approach does not have this problem because he keeps\n> track of all entries under $GIT_WORK_TREE and knows which files are\n> truely new. But I don't really want to keep the whole file list around,\n> especially when watchman already manages the same list.\n> \n> So we got a few options:\n> \n> 1) Convince watchman devs to add something to make it work\n> \n> 2) Fork watchman\n> \n> 3) Make another daemon to keep file list around, or put it in a shared\n>    memory.\n> \n> 4) Move David's watchman series forward (and maybe make use of shared\n>    mem for fs_cache).\n> \n> 5) Go with something similar to the patch below and accept untracked\n>    cache performance degrades from time to time\n> \n> 6) ??\n> \n> I'm working on 1). 2) is just bad taste, listed for completeness\n> only. If we go with 3) and watchman starts to support Windows (seems\n> to be in their plan), we'll need to rework some how. And I really\n> don't like 3)\n> \n> If 1-3 does not work out, we're left without 4) and 5). We could\n> support both, but proobably not worth the code complexity and should\n> just go with one.\n> \n> And if we go with 4) we should probably think of dropping untracked\n> cache if watchman will support Windows in the end. 4) also has another\n> advantage over untracked cache, that it could speed up listing ignored\n> files as well as untracked files.\n> \n> Comments?\n> \n[remove the patch]\nFrom a Git user perspective it could be good to have something like this:\n\na) git status -u\nb) git status -uno\nc) git status -umtime\nd) git status -uwatchman\n\nWe know that a) and b) already exist.\nc) Can be convenient to have, in order to do benchmarking and testing.\n  When the UNTR extension is not found, Git can give an error,\n  saying something like this:\n  No mtime information found, use \"git update-index --untracked-cache\"\nd) does not yet exist\n\nOf course we may want to configure the default for \"git status\" in a default variable,\nlike status.findUntrackedFiles, which can be empty \"\", \"mtime\" or \"watchman\",\nand we may add other backends later.\n\nA short test showed that watchman compiles under Mac OS.\nThe patch did not compile out of the box (both Git and watchman declare\nthere own version of usage(), some C99 complaints from the compiler in watchman,\nnothing that can not be fixed easily)\n\n\nI will test the mtime patch under networked file systems the next weeks.\n\n\nThe short version:\nGo with c), d) then 5) until we have something better :-) \n"},{"id":"251812","messageId":"CACsJy8AKsvL2XcBMGG1Jy_W2KaOCuYm16Ffk529KDOARr68XNQ@mail.gmail.com","threadId":"37931","inReplyTo":"54643C30.6010204@web.de","subject":"Re: [RFC] On watchman support","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-11-13T12:22:48Z","receivedAt":"2014-11-13T12:22:48Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Nov 13, 2014 at 12:05 PM, Torsten Bögershausen <tboegi@web.de> wrote:\n> From a Git user perspective it could be good to have something like this:\n>\n> a) git status -u\n> b) git status -uno\n> c) git status -umtime\n> d) git status -uwatchman\n>\n> We know that a) and b) already exist.\n> c) Can be convenient to have, in order to do benchmarking and testing.\n>   When the UNTR extension is not found, Git can give an error,\n>   saying something like this:\n>   No mtime information found, use \"git update-index --untracked-cache\"\n> d) does not yet exist\n>\n> Of course we may want to configure the default for \"git status\" in a default variable,\n> like status.findUntrackedFiles, which can be empty \"\", \"mtime\" or \"watchman\",\n> and we may add other backends later.\n\nWhile \"git status\" is in the spotlight, these optimizations have wider\nimpact. Faster index read/refresh/write helps the majority of\ncommands. Faster untracked listing hits git-status, git-add,\ngit-commit -A... This is why I go with environment variable for\ntemporarily disabling something, or we'll need many config and command\nline options, one per command.\n\n> A short test showed that watchman compiles under Mac OS.\n> The patch did not compile out of the box (both Git and watchman declare\n> there own version of usage(), some C99 complaints from the compiler in watchman,\n> nothing that can not be fixed easily)\n\nYeah it's not perfect. It's mainly to show speeding up refresh with\nwatchman could be done easily and with low impact\n\n> I will test the mtime patch under networked file systems the next weeks.\n\nHmm.. you remind me mtime series may have this as an advantage over watchman..\n-- \nDuy\n"},{"id":"251930","messageId":"5466FFBC.6020207@web.de","threadId":"37931","inReplyTo":"CACsJy8AKsvL2XcBMGG1Jy_W2KaOCuYm16Ffk529KDOARr68XNQ@mail.gmail.com","subject":"Re: [RFC] On watchman support","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2014-11-15T07:24:44Z","receivedAt":"2014-11-15T07:24:44Z","isPatch":false,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 11/13/2014 01:22 PM, Duy Nguyen wrote:\n> On Thu, Nov 13, 2014 at 12:05 PM, Torsten Bögershausen <tboegi@web.de> wrote:\n>> From a Git user perspective it could be good to have something like this:\n>>\n>> a) git status -u\n>> b) git status -uno\n>> c) git status -umtime\n>> d) git status -uwatchman\n>>\n>> We know that a) and b) already exist.\n>> c) Can be convenient to have, in order to do benchmarking and testing.\n>>   When the UNTR extension is not found, Git can give an error,\n>>   saying something like this:\n>>   No mtime information found, use \"git update-index --untracked-cache\"\n>> d) does not yet exist\n>>\n>> Of course we may want to configure the default for \"git status\" in a default variable,\n>> like status.findUntrackedFiles, which can be empty \"\", \"mtime\" or \"watchman\",\n>> and we may add other backends later.\n> While \"git status\" is in the spotlight, these optimizations have wider\n> impact. Faster index read/refresh/write helps the majority of\n> commands. Faster untracked listing hits git-status, git-add,\n> git-commit -A... This is why I go with environment variable for\n> temporarily disabling something, or we'll need many config and command\n> line options, one per command.\n>\n>> A short test showed that watchman compiles under Mac OS.\n>> The patch did not compile out of the box (both Git and watchman declare\n>> there own version of usage(), some C99 complaints from the compiler in watchman,\n>> nothing that can not be fixed easily)\n> Yeah it's not perfect. It's mainly to show speeding up refresh with\n> watchman could be done easily and with low impact\n>\n>> I will test the mtime patch under networked file systems the next weeks.\n\n\nThinks become to get a little bit clearer.\nWhat I can understand is that we have 2 different \"update-helpers\" for Git,\nthanks for that.\n\njust in case there is re-roll, does the following makes sense:\nWe want to enable them (probably only one at a time) either by command line or\npersistent in a repo.\n\nAs I think we have 2 different update helpers\n(and may be more in the future)\nGIT_UPDATE_HELPER=dirmtime git status\nGIT_UPDATE_HELPER=watchman git status\nGIT_UPDATE_HELPER=none git status\n\nof course we want to be able to configure it:\ngit config core.updatehelper dirmtime\n\n\nAfter configuring we may want to override it:\nGIT_UPDATE_HELPER=none git status\nor\ngit -c core.updatehelper=none status\n\n> Hmm.. you remind me mtime series may have this as an advantage over watchman..\nI had the time to do a short test, sharing a copy of git.git under NFS:\nThe time for git status dropped from 0.4 seconds to 0.15 seconds or so.\nVery nice.\nThe next test will be to share the same repo under samba to Windows\nand Mac OS and see how this works.\n"},{"id":"252018","messageId":"1416270336.13653.23.camel@leckie","threadId":"37931","inReplyTo":"20141111124901.GA6011@lanh","subject":"Re: [RFC] On watchman support","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2014-11-18T00:25:36Z","receivedAt":"2014-11-18T00:25:36Z","isPatch":false,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Tue, 2014-11-11 at 19:49 +0700, Duy Nguyen wrote:\n> I've come to the last piece to speed up \"git status\", watchman\n> support. And I realized it's not as good as I thought.\n> \n> Watchman could be used for two things: to avoid refreshing the index,\n> and to avoid searching for ignored files. The first one can be done\n> (with the patch below as demonstration). And it should keep refresh\n> cost to near zero in the best case, the cost is proportional to the\n> number of modified files.\n> \n> For avoiding searching for ignored files. My intention was to build on\n> top of untracked cache. If watchman can tell me what files are added\n> or deleted since last observed time, then I can invalidate just\n> directories that contain them, or even better, calculate ignore status\n> for those files only.\n> \n> This is important because in reality compilers and editors tend to\n> update files by creating a new version then rename them, updating\n> directory mtime and invalidating untracked cache as a consequence. As\n> you edit more files (or your rebuild touches more dirs), untracked\n> cache performance drops (until the next \"git status\"). The numbers I\n> posted so far are the best case.\n> \n> The problem with watchman is it cannot tell me \"new\" files since the\n> last observed time (let's say 'T'). If a file exists at 'T', gets\n> deleted then recreated, then watchman tells me it's a new file. I want\n> to separate those from ones that do not exist before 'T'.\n> \n> David's watchman approach does not have this problem because he keeps\n> track of all entries under $GIT_WORK_TREE and knows which files are\n> truely new. But I don't really want to keep the whole file list around,\n> especially when watchman already manages the same list.\n> \n> So we got a few options:\n> \n> 1) Convince watchman devs to add something to make it work\n\nBased on the thread on the watchman github it looks like this won't\nhappen. \n\n> 2) Fork watchman\n> \n> 3) Make another daemon to keep file list around, or put it in a shared\n>    memory.\n> \n> 4) Move David's watchman series forward (and maybe make use of shared\n>    mem for fs_cache).\n> \n> 5) Go with something similar to the patch below and accept untracked\n>    cache performance degrades from time to time\n> \n> 6) ??\n> \n> I'm working on 1). 2) is just bad taste, listed for completeness\n> only. If we go with 3) and watchman starts to support Windows (seems\n> to be in their plan), we'll need to rework some how. And I really\n> don't like 3)\n> \n> If 1-3 does not work out, we're left without 4) and 5). We could\n> support both, but proobably not worth the code complexity and should\n> just go with one.\n> \n> And if we go with 4) we should probably think of dropping untracked\n> cache if watchman will support Windows in the end. 4) also has another\n> advantage over untracked cache, that it could speed up listing ignored\n> files as well as untracked files.\n> \n> Comments?\n\nI don't think it would be impossible to add Windows support to watchman;\nthe necessary functions exist, although I don't know how well they work.\nMy experience with watchman is that it is something of a stress test of\na filesystem's notification layer.  It has exposed bugs in inotify, and\ncaused system instability on OS X.\n\nMy patches are not the world's most beautiful, but they do work.  I\nthink some improvement might be possible by keeping info about tracked\nfiles in the index, and only storing the tree of ignored and untracked\nfiles separately.  But I have not thought this through fully.  In any\ncase, making use of shared memory for the fs_cache (as some of your\nother patches do for the index) would definitely save time.\n"},{"id":"252079","messageId":"CACsJy8BfxP7KF1XF29BOgC6XhO8iAy-ycEoLkDG5rn6TYH_DrA@mail.gmail.com","threadId":"37931","inReplyTo":"1416270336.13653.23.camel@leckie","subject":"Re: [RFC] On watchman support","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-11-18T10:48:50Z","receivedAt":"2014-11-18T10:48:50Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Nov 18, 2014 at 7:25 AM, David Turner <dturner@twopensource.com> wrote:\n>> So we got a few options:\n>>\n>> 1) Convince watchman devs to add something to make it work\n>\n> Based on the thread on the watchman github it looks like this won't\n> happen.\n\nYeah. I came to the conclusion that I needed an extra daemon. And\nbecause I would need an extra daemon anyway to speed up index read\ntime, that one could be used for caching something else like watchman.\nIt works out quite nice: because watchman is not tied to the main\n'git' binary, people don't need libwatchman and libjansson by default.\nWhen people want watchman, they can install an extra package that\nincludes the index-helper and its dependencies. This only matters for\nbinary-based distros of course.\n\n>> Comments?\n>\n> I don't think it would be impossible to add Windows support to watchman;\n> the necessary functions exist, although I don't know how well they work.\n> My experience with watchman is that it is something of a stress test of\n> a filesystem's notification layer.  It has exposed bugs in inotify, and\n> caused system instability on OS X.\n\nThe way i'm adding watchman to index-helper should work on Windows as\nwell, IPC is really simple. But let's wait and see.\n\n> My patches are not the world's most beautiful, but they do work.  I\n> think some improvement might be possible by keeping info about tracked\n> files in the index, and only storing the tree of ignored and untracked\n> files separately.  But I have not thought this through fully.  In any\n> case, making use of shared memory for the fs_cache (as some of your\n> other patches do for the index) would definitely save time.\n\nBy the way, what happened to your sse optimization in refs.c? I see\nit's reverted but I didn't follow closely to know why. Or will you go\nwith cityhash now.. I ask because you have another sse optimization\nfor hashmap on your watchman branch and that could reduce init time\nfor name-hash. Name-hash is used often on case-insensitive fs (less\noften on case-sensitive fs).\n\nI did a simple test and your optimization could init name-hash (on\nwebkit) in 35ms, while unmodified hashmap took 88ms. Loading index on\nthis machine took 360ms for reference (probably down too 100ms with\nindex-helper running, when that 88ms starts to become significant).\n-- \nDuy\n"},{"id":"252098","messageId":"1416334360.27401.10.camel@leckie","threadId":"37931","inReplyTo":"CACsJy8BfxP7KF1XF29BOgC6XhO8iAy-ycEoLkDG5rn6TYH_DrA@mail.gmail.com","subject":"Re: [RFC] On watchman support","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2014-11-18T18:12:40Z","receivedAt":"2014-11-18T18:12:40Z","isPatch":false,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Tue, 2014-11-18 at 17:48 +0700, Duy Nguyen wrote:\n> > My patches are not the world's most beautiful, but they do work.  I\n> > think some improvement might be possible by keeping info about tracked\n> > files in the index, and only storing the tree of ignored and untracked\n> > files separately.  But I have not thought this through fully.  In any\n> > case, making use of shared memory for the fs_cache (as some of your\n> > other patches do for the index) would definitely save time.\n> \n> By the way, what happened to your sse optimization in refs.c? I see\n> it's reverted but I didn't follow closely to know why. \n\nI don't know why either -- it works just fine.  There was a bug, but I\nfixed it.  Junio?\n\n> Or will you go\n> with cityhash now.. I ask because you have another sse optimization\n> for hashmap on your watchman branch and that could reduce init time\n> for name-hash. Name-hash is used often on case-insensitive fs (less\n> often on case-sensitive fs).\n\nCityhash would be better, because it has actual engineering effort put\ninto it; what I did on my branch is a hack that happens to work\ndecently.  As the comment notes, I did not spend much effort on tuning\nmy implementation.  Also, Cityhash doesn't require SSE, so it's more\nportable.\n\n> I did a simple test and your optimization could init name-hash (on\n> webkit) in 35ms, while unmodified hashmap took 88ms. Loading index on\n> this machine took 360ms for reference (probably down too 100ms with\n> index-helper running, when that 88ms starts to become significant).\n\nOK, that sounds like a big win.  \n"},{"id":"252128","messageId":"xmqqioicut32.fsf@gitster.dls.corp.google.com","threadId":"37931","inReplyTo":"1416334360.27401.10.camel@leckie","subject":"Re: [RFC] On watchman support","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-18T20:55:29Z","receivedAt":"2014-11-18T20:55:29Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Turner <dturner@twopensource.com> writes:\n\n> On Tue, 2014-11-18 at 17:48 +0700, Duy Nguyen wrote:\n>> > My patches are not the world's most beautiful, but they do work.  I\n>> > think some improvement might be possible by keeping info about tracked\n>> > files in the index, and only storing the tree of ignored and untracked\n>> > files separately.  But I have not thought this through fully.  In any\n>> > case, making use of shared memory for the fs_cache (as some of your\n>> > other patches do for the index) would definitely save time.\n>> \n>> By the way, what happened to your sse optimization in refs.c? I see\n>> it's reverted but I didn't follow closely to know why. \n>\n> I don't know why either -- it works just fine.  There was a bug, but I\n> fixed it.  Junio?\n\nI vaguely recall that the reason why we dropped it was because it\nwas too much code churn in an area that was being worked on in\nparallel, but you may need to go back to the list archive for\ndetails.\n"},{"id":"252132","messageId":"1416345123.27401.11.camel@leckie","threadId":"37931","inReplyTo":"xmqqioicut32.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC] On watchman support","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2014-11-18T21:12:03Z","receivedAt":"2014-11-18T21:12:03Z","isPatch":false,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Tue, 2014-11-18 at 12:55 -0800, Junio C Hamano wrote:\n> David Turner <dturner@twopensource.com> writes:\n> \n> > On Tue, 2014-11-18 at 17:48 +0700, Duy Nguyen wrote:\n> >> > My patches are not the world's most beautiful, but they do work.  I\n> >> > think some improvement might be possible by keeping info about tracked\n> >> > files in the index, and only storing the tree of ignored and untracked\n> >> > files separately.  But I have not thought this through fully.  In any\n> >> > case, making use of shared memory for the fs_cache (as some of your\n> >> > other patches do for the index) would definitely save time.\n> >> \n> >> By the way, what happened to your sse optimization in refs.c? I see\n> >> it's reverted but I didn't follow closely to know why. \n> >\n> > I don't know why either -- it works just fine.  There was a bug, but I\n> > fixed it.  Junio?\n> \n> I vaguely recall that the reason why we dropped it was because it\n> was too much code churn in an area that was being worked on in\n> parallel, but you may need to go back to the list archive for\n> details.\n\nOK, in that case I'll try to remember to reroll it once the rest of the\nrefs stuff lands.\n"},{"id":"252136","messageId":"xmqqwq6std27.fsf@gitster.dls.corp.google.com","threadId":"37931","inReplyTo":"1416345123.27401.11.camel@leckie","subject":"Re: [RFC] On watchman support","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-18T21:26:56Z","receivedAt":"2014-11-18T21:26:56Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Turner <dturner@twopensource.com> writes:\n\n> On Tue, 2014-11-18 at 12:55 -0800, Junio C Hamano wrote:\n>> I vaguely recall that the reason why we dropped it was because it\n>> was too much code churn in an area that was being worked on in\n>> parallel, but you may need to go back to the list archive for\n>> details.\n>\n> OK, in that case I'll try to remember to reroll it once the rest of the\n> refs stuff lands.\n\nSure.  But I would much prefer to see us explore an arch independent\noptimisation of the caller before starting to micro-optimize a leaf\nfunction.  \n\nIt is not check_refname_format() that is the real problem. It's the\nfact that we do O(# of refs) work whenever we have to access the\npacked-refs file. check_refname_format() is part of that, surely,\nbut so is reading the file, creating all of the refname structs in\nmemory, etc. (credit to peff@).\n"},{"id":"252164","messageId":"20141119014600.GA2337@peff.net","threadId":"37931","inReplyTo":"xmqqwq6std27.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC] On watchman support","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-11-19T01:46:00Z","receivedAt":"2014-11-19T01:46:00Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 18, 2014 at 01:26:56PM -0800, Junio C Hamano wrote:\n\n> It is not check_refname_format() that is the real problem. It's the\n> fact that we do O(# of refs) work whenever we have to access the\n> packed-refs file. check_refname_format() is part of that, surely,\n> but so is reading the file, creating all of the refname structs in\n> memory, etc. (credit to peff@).\n\nYeah, I'd agree very much with that. I am not sure if I am cc'd here\nbecause of my general complaining about packed-refs, or if I have said\nsomething clever on the subject.\n\nI did implement at one point a packed-refs reader that does a binary\nsearch on the mmap'd packed-refs file, and can return a single value or\neven locate the first entry matching a prefix (like \"refs/tags/\") and\niterate until we're out of the prefix. Unfortunately this runs very\ncontrary to the caching design of the refs.c code. It is focused on\ncaching _loose_ refs, where we may read an outer directory (like\n\"refs/\"), and would like to avoid descending into an inner directory\n(likes \"refs/foo/\") unless we are interested in what is in it. But\ncaching partial reads of packed-refs like this is \"inside out\"; we might\nread all of \"refs/tags/*\", but have no clue what else is in \"refs/\". So\nintegrating it into refs.c would take pretty major surgery.\n\n-Peff\n"},{"id":"252182","messageId":"CAHVLzcnb6_rYPqKNFvnqrnwuToCeRp8NPY31Y-cbOEyY=wYvvg@mail.gmail.com","threadId":"37931","inReplyTo":"1416270336.13653.23.camel@leckie","subject":"Re: [RFC] On watchman support","fromName":"Paolo Ciarrocchi","fromEmail":"paolo.ciarrocchi@gmail.com","sentAt":"2014-11-19T15:26:19Z","receivedAt":"2014-11-19T15:26:19Z","isPatch":false,"sender":{"key":"paolo.ciarrocchi@gmail.com","avatar":null},"body":"On Tue, Nov 18, 2014 at 1:25 AM, David Turner <dturner@twopensource.com> wrote:\n>\n> My patches are not the world's most beautiful, but they do work.\n\nOut of curiosity: do you run the patches at twitter?\n\nThanks.\n\n-- Paolo\n\n\n-- \nPaolo\n"},{"id":"252184","messageId":"1416415425.14486.1.camel@leckie","threadId":"37931","inReplyTo":"CAHVLzcnb6_rYPqKNFvnqrnwuToCeRp8NPY31Y-cbOEyY=wYvvg@mail.gmail.com","subject":"Re: [RFC] On watchman support","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2014-11-19T16:43:45Z","receivedAt":"2014-11-19T16:43:45Z","isPatch":false,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Wed, 2014-11-19 at 16:26 +0100, Paolo Ciarrocchi wrote:\n> On Tue, Nov 18, 2014 at 1:25 AM, David Turner <dturner@twopensource.com> wrote:\n> >\n> > My patches are not the world's most beautiful, but they do work.\n> \n> Out of curiosity: do you run the patches at twitter?\n\nAn increasing number of us do, yes. \n"},{"id":"252656","messageId":"CACsJy8DayFy83JijrkgST5rAbNsst-dgqaP-ebpWXoGKPtp7sA@mail.gmail.com","threadId":"37931","inReplyTo":"1416334360.27401.10.camel@leckie","subject":"Re: [RFC] On watchman support","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-11-28T11:13:24Z","receivedAt":"2014-11-28T11:13:24Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Nov 19, 2014 at 1:12 AM, David Turner <dturner@twopensource.com> wrote:\n>> Or will you go\n>> with cityhash now.. I ask because you have another sse optimization\n>> for hashmap on your watchman branch and that could reduce init time\n>> for name-hash. Name-hash is used often on case-insensitive fs (less\n>> often on case-sensitive fs).\n>\n> Cityhash would be better, because it has actual engineering effort put\n> into it; what I did on my branch is a hack that happens to work\n> decently.  As the comment notes, I did not spend much effort on tuning\n> my implementation.  Also, Cityhash doesn't require SSE, so it's more\n> portable.\n\nCityhash looks less appealing to me. For one thing it's C++ so linking\nto C can't be done. I could add a few \"extern \"C\"\" to make it work.\nBut if we plan to support it eventually, cityhash must support C out\nof the box.\n\nThen cityhash does not support case-insensitive hashing. I had to make\na CityHash32i version based on CityHash32. It's probably my bugs\nthere, but performance is worse (~120ms) than original hashmap.c\n(90ms). Enabling sse4.2 helps a bit, but still worse. Using the\ncase-sensitive version in place for memihash and strihash does make\ncityhash win over hashmap.c, around 50ms (with or without sse4.2). But\nthat's still not as good as your version (~35ms)..\n-- \nDuy\n"},{"id":"252808","messageId":"1417466750.20544.2.camel@leckie","threadId":"37931","inReplyTo":"CACsJy8DayFy83JijrkgST5rAbNsst-dgqaP-ebpWXoGKPtp7sA@mail.gmail.com","subject":"Re: [RFC] On watchman support","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2014-12-01T20:45:50Z","receivedAt":"2014-12-01T20:45:50Z","isPatch":false,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Fri, 2014-11-28 at 18:13 +0700, Duy Nguyen wrote:\n> On Wed, Nov 19, 2014 at 1:12 AM, David Turner <dturner@twopensource.com> wrote:\n> >> Or will you go\n> >> with cityhash now.. I ask because you have another sse optimization\n> >> for hashmap on your watchman branch and that could reduce init time\n> >> for name-hash. Name-hash is used often on case-insensitive fs (less\n> >> often on case-sensitive fs).\n> >\n> > Cityhash would be better, because it has actual engineering effort put\n> > into it; what I did on my branch is a hack that happens to work\n> > decently.  As the comment notes, I did not spend much effort on tuning\n> > my implementation.  Also, Cityhash doesn't require SSE, so it's more\n> > portable.\n> \n> Cityhash looks less appealing to me. For one thing it's C++ so linking\n> to C can't be done. I could add a few \"extern \"C\"\" to make it work.\n> But if we plan to support it eventually, cityhash must support C out\n> of the box.\n> \n> Then cityhash does not support case-insensitive hashing. I had to make\n> a CityHash32i version based on CityHash32. It's probably my bugs\n> there, but performance is worse (~120ms) than original hashmap.c\n> (90ms). Enabling sse4.2 helps a bit, but still worse. Using the\n> case-sensitive version in place for memihash and strihash does make\n> cityhash win over hashmap.c, around 50ms (with or without sse4.2). But\n> that's still not as good as your version (~35ms)..\n\nCan you post your CityHash32i?  \n\nHave you tried this C port of Cityhash?\n\nhttps://github.com/santeri-io/cityhash-c\n"}]}